modularization by evaleev · Pull Request #477 · ValeevGroup/SeQuant

evaleev · 2026-02-03T22:41:13Z

along the lines of #391, prompted by devious OpenMP compilation injection into TA breaking one of the tests. This includes not only logical reorganization into proto-C++-modules, but some physical layout adjustments to more cleanly separate symbolic core from eval from export. @Krzmbrzl have a peek.

Copilot description follows.

This pull request significantly restructures the CMake build system for the SeQuant project, introducing a modular, layered approach to library targets. The changes partition the codebase into logical components (symbolic, evaluation, export, MBPT domain, etc.), improve dependency management, and update installation and configuration logic accordingly. Additionally, some header inclusions and typos are fixed, and documentation is updated for clarity.

CMake build system modularization and improvements:

Refactored CMakeLists.txt to define separate CMake targets for each logical layer: SeQuant-symb (symbolic core), SeQuant-eval (evaluation framework), SeQuant-eval-ta (TiledArray backend), SeQuant-eval-btas (BTAS backend), SeQuant-export (export/codegen), SeQuant-core (version info, links symbolic+export), and SeQuant-mbpt (MBPT domain code). An umbrella SeQuant INTERFACE target is provided for backwards compatibility. [1] [2] [3] [4] [5] [6]
Updated installation logic to install all new targets and ensure all expected imported targets exist, with improved error messages in sequant-config.cmake.in.
Improved handling of include-what-you-use (IWYU) by applying it to all new targets, not just the monolithic one.

Code organization and header updates:

Moved optimization code from SeQuant/core/optimize.* to SeQuant/core/eval/optimize.* and updated all includes accordingly. [1] [2] [3] [4] [5]
Fixed a typo in documentation: changed aproximate_size() to approximate_size() in SeQuant/core/eval/optimize.hpp. [1] [2] [3]

Documentation:

Clarified operator label case in doc/user/guide/operator.rst ("T" or "Λ" → "t" or "λ").

this test would not compile because TA inherited compile OpenMP dependence from blaspp (?) .. and OpenMP is not quite C++.

…sumers (incl. SQ tests) on TA/BTAS ... and to model the module structure described MS1.

Copilot

Pull request overview

This PR refactors the SeQuant CMake build system from a monolithic structure to a modular, layered architecture. The changes partition the codebase into logical components (symbolic core, evaluation framework, export/codegen, MBPT domain, and backend-specific modules), improve dependency management, and update installation/configuration logic. Additionally, optimization code is relocated from SeQuant/core/optimize.* to SeQuant/core/eval/optimize.*, with all includes updated throughout the codebase, and several typos are corrected.

Changes:

Introduced modular CMake targets: SeQuant-symb, SeQuant-eval, SeQuant-eval-ta, SeQuant-eval-btas, SeQuant-export, SeQuant-core, and SeQuant-mbpt, with an umbrella SeQuant INTERFACE target for backwards compatibility
Reorganized optimization code from core/optimize to core/eval/optimize directory structure with corresponding header path updates across the entire codebase
Restructured unit tests into separate OBJECT libraries aligned with the new modular targets, improving build organization and compilation efficiency

Reviewed changes

Copilot reviewed 19 out of 21 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
CMakeLists.txt	Defines new modular library targets with explicit source lists and dependencies; updates installation and IWYU configuration
cmake/sequant-config.cmake.in	Updates target verification logic to check for all new modular targets
tests/unit/CMakeLists.txt	Restructures tests into OBJECT libraries per module; maintains conditional backend compilation
utilities/external-interface/*.cpp	Updates includes from `core/optimize` to `core/eval/optimize`
tests/unit/test_*.cpp	Updates includes to reflect new header locations
tests/integration/eval/calc_info.hpp	Updates include path for optimize header
SeQuant/core/eval/optimize/*.{hpp,cpp}	Adds new header files for modularized optimization code
SeQuant/core/expressions/result_expr.cpp	Removes unnecessary optimize.hpp include
SeQuant/domain/mbpt/spin.cpp	Updates include path for optimize functionality
tests/unit/gwt.hpp	Changes from structured binding to explicit reference extraction (compatibility/style)
doc/user/guide/operator.rst	Clarifies operator label case convention
SeQuant/core/eval/optimize.hpp	Corrects spelling: "aproximate_size()" → "approximate_size()"

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-02-03T22:48:26Z

CMakeLists.txt

+##########################
+add_library(SeQuant-eval ${SeQuant_eval_src})
+target_link_libraries(SeQuant-eval PUBLIC SeQuant-symb)
+target_compile_definitions(SeQuant-eval PUBLIC SEQUANT_HAS_EVAL_BACKENDS=1)


The SEQUANT_HAS_EVAL_BACKENDS macro is defined on SeQuant-eval but is not used anywhere in the codebase. This appears to be vestigial and can be removed for clarity. The specific backend macros (SEQUANT_HAS_TILEDARRAY and SEQUANT_HAS_BTAS) are sufficient and actually used.

Suggested change

target_compile_definitions(SeQuant-eval PUBLIC SEQUANT_HAS_EVAL_BACKENDS=1)

Krzmbrzl

I know that a lot of the "could be private" comments are not inherently new due to changes made in this PR but I feel like the strive for a more modularized setup makes these kinds of things much more important.

Krzmbrzl · 2026-02-04T09:06:50Z

cmake/sequant-config.cmake.in

For modularized CMake projects, it is often expected that I can specify what components I want when using find_package. This allows using some components without having to worry about providing the dependencies of other, unused modules.

In fact, this would be the biggest use of a modularized setup (only having to provide dependencies for things I actually need)

See e.g. TheLartians/PackageProject.cmake#48 for how this could be done somewhat generically

I propose we push this to a separate PR. Once the project is compiled loading all components vs loading only select components presumably only costs the end user the extra costs of cmake configuration; as long as they link against the target they want they will no depend on other unused components and the targets they loaded. Yes, loading more stuff costs more and increases the chance of things breaking, but ... one step at a time.

Krzmbrzl · 2026-02-04T09:09:04Z

cmake/sequant-config.cmake.in

+  set(grandTargetList SeQuant-symb SeQuant-eval SeQuant-export SeQuant-core SeQuant-mbpt SeQuant)
+  if (SEQUANT_HAS_TILEDARRAY)
+    list(APPEND grandTargetList SeQuant-eval-ta)
  endif()
+  if (SEQUANT_HAS_BTAS)
+    list(APPEND grandTargetList SeQuant-eval-btas)
+  endif()
+  foreach(_target ${grandTargetList})
+    if(NOT TARGET SeQuant::${_target})
+      message(FATAL_ERROR "expected SeQuant::${_target} among imported SeQuant targets")
+    endif()
+  endforeach()


I would suggest dropping the additional SeQuant in the component name such that the imported targets become SeQuant::core, SeQuant::eval etc.

Ideally, yes ... but to be able to support building from source in a large component graph the targets need to be namespaced (yes, use EP/superbuild to namespace ... No thx, done that, still do for some deps that we don't control, but FC is far more workable). Currently for consistency we even build-tree targets get exported with SeQuant:: namespace, to make their consumption easier. But there doesn't seem to be an easy way to tell {install,export}(EXPORT to rename in a custom way ... I'm sure someone somewhere solved this problem ... hints/proposals?

Iirc, you only need to set the EXPORT_NAME property on targets: https://cmake.org/cmake/help/latest/prop_tgt/EXPORT_NAME.html

bingo, thanks!

Krzmbrzl · 2026-02-04T09:15:11Z

CMakeLists.txt

+        SeQuant/core/eval/optimize.hpp
+        SeQuant/core/eval/optimize/common_subexpression_elimination.hpp
+        SeQuant/core/eval/optimize/fusion.cpp
+        SeQuant/core/eval/optimize/fusion.hpp
+        SeQuant/core/eval/optimize/optimize.cpp


optimize shouldn't be part of the eval module. It has a dependency on the eval node class, but that is currently an implementation detail. Semantically, none of the optimization functions are tied to evaluation.

I struggled with this one. I disagree though. Optimize implies an objective function and that means execution model etc. All of this logically belongs to eval.

@bimalgaudel thoughts?

I think some optimization functionality can be moved out of eval to expr while others cannot. Anything that involves Expr rewrite can be moved to expr e.g. single-term optimization (STO) with a generic cost function or fusion of terms. However, whenever runtime intermediate reuse is to be figured out or the evaluation order for individual terms from a large sum is to be determined, that logic probably not suitable for expr module and could be handled from eval.

But even single term optimization already assumes something about the backend (e.g. it only supports binary contractions) ... I had the same instinct at first, but the more I thought the less I was convinced. I don't see a significant downside to having optimize with its friends in eval.

Most downstream users will use SeQuant, so I think this will impact few.

I would argue that wanting to have binary contractions is the de-facto standard that people will want even before considering what exact backend they will end up using.

I'm not sure ... in numpy.einsum world everyone just wants TN evaluation.

Semantics - single-term-optimization and identification of common subexpressions don't have anything to do with driving a numerical backend from SeQuant. They both operate on the symbolic level (albeit in their implementation they actually do use an evaluation tree - but that's an implementation detail, I would say)

The problem is that this implementation detail impacts modularization. optimize must be together or downstream from binarize. So we must compromise somewhere. Either we abandon symb and eval and merge them into core. But then keeping SeQuant::eval::{backend} modules is weird because there is no SeQuant::eval. And separating the backend-specific modules is necessary.

I do not see how to keep optimize in symb without making more significant sacrifices. If you have better ideas then make a proposal.

I would probably just make optimize its own module. Currently it's only STO and CSE but longer term there js certainly possibilities for expanding optimization functionality.

where to draw the line between optimize and eval to avoid cyclic dep? optimize depend on eval? I don't know if the users will grok that better ...

I'd say we bundle exactly as we currently have the subdirectories set up. Everything in core/optimize goes into optimize and everything from core/eval goes into eval.

optimize depend on eval? I don't know if the users will grok that better ...

This dependency is more or less transparent to downstream users - as long as we separate between depending on eval and eval::ta/eval::btas. The main user-facing irritation with the current state of this PR is that I have to include files from the eval subdirectory when accessing optimization functionality. This would be gone if we make optimize its own module…

OK, yes, after typing I thought some more and realized probably that's what you meant ... Optimize can be viewed as IR getting emitted back into SeQuant language, rather than evaluated or exported. So its dependence on eval is natural from that perspective. Skipping IR for the purposes of optimize is possible, but violates DRY.

OK, sounds good. I'll introduce optimize module.

Krzmbrzl · 2026-02-04T09:18:56Z

CMakeLists.txt

+# SeQuant-symb: core symbolic layer
+##########################
+add_library(SeQuant-symb ${SeQuant_symb_src})
+target_compile_definitions(SeQuant-symb PUBLIC SEQUANT_ASSERT_BEHAVIOR_=${SEQUANT_ASSERT_BEHAVIOR})


This can be PRIVATE if we move the implementation of assert_failed into a cpp file

Krzmbrzl · 2026-02-04T09:20:29Z

CMakeLists.txt

+target_compile_definitions(SeQuant-symb PUBLIC SEQUANT_ASSERT_BEHAVIOR_=${SEQUANT_ASSERT_BEHAVIOR})
 if (SEQUANT_CONTEXT_MANIPULATION_THREADSAFE)
-  target_compile_definitions(SeQuant PUBLIC SEQUANT_CONTEXT_MANIPULATION_THREADSAFE)
+    target_compile_definitions(SeQuant-symb PUBLIC SEQUANT_CONTEXT_MANIPULATION_THREADSAFE)


This could be PRIVATE if we also define it for the mbpt component

Krzmbrzl · 2026-02-04T09:39:48Z

CMakeLists.txt

+    target_link_libraries(SeQuant INTERFACE SeQuant-eval-btas)
+endif()
+if (SEQUANT_EVAL_TRACE)
+    target_compile_definitions(SeQuant INTERFACE SEQUANT_EVAL_TRACE=1)


Shouldn't this be defined on the eval module 👀

Krzmbrzl · 2026-02-04T09:41:13Z

CMakeLists.txt

+        set_property(TARGET SeQuant-symb PROPERTY CXX_INCLUDE_WHAT_YOU_USE ${iwyu_options_and_path})
+        set_property(TARGET SeQuant-mbpt PROPERTY CXX_INCLUDE_WHAT_YOU_USE ${iwyu_options_and_path})
+        set_property(TARGET SeQuant-core PROPERTY CXX_INCLUDE_WHAT_YOU_USE ${iwyu_options_and_path})
+        set_property(TARGET SeQuant-eval PROPERTY CXX_INCLUDE_WHAT_YOU_USE ${iwyu_options_and_path})
+        set_property(TARGET SeQuant-export PROPERTY CXX_INCLUDE_WHAT_YOU_USE ${iwyu_options_and_path})
+        if (SEQUANT_HAS_TILEDARRAY)
+            set_property(TARGET SeQuant-eval-ta PROPERTY CXX_INCLUDE_WHAT_YOU_USE ${iwyu_options_and_path})
+        endif()
+        if (SEQUANT_HAS_BTAS)
+            set_property(TARGET SeQuant-eval-btas PROPERTY CXX_INCLUDE_WHAT_YOU_USE ${iwyu_options_and_path})
+        endif()


We could use a loop over all active SeQuant components to simplify this code (and also the install setup)

Krzmbrzl · 2026-02-04T09:43:35Z

tests/unit/CMakeLists.txt

+    Catch2::Catch2
+    dtl::dtl
+    Boost::boost
+    Eigen3::Eigen)


I believe Eigen is only needed for the export validation tests, which aren't part of this object library.

Krzmbrzl · 2026-02-04T09:45:35Z

tests/unit/CMakeLists.txt

        target_compile_definitions(unit_tests-sequant PRIVATE
            SEQUANT_HAS_NUMPY_FOR_VALIDATION=1)


I don't think this is necessary for the main test executable

Krzmbrzl · 2026-02-04T09:46:53Z

tests/unit/CMakeLists.txt

Dependency handling could be more scoped - currently it seems all dependencies are added to all module tests.

evaleev added 3 commits February 3, 2026 12:13

dox typos

91c22f6

start of the saga: avoid needless structured binding capture by a lambda

80a8e9f

this test would not compile because TA inherited compile OpenMP dependence from blaspp (?) .. and OpenMP is not quite C++.

modularization of SeQuant to get rid of unnecessary dependence of con…

48abc30

…sumers (incl. SQ tests) on TA/BTAS ... and to model the module structure described MS1.

evaleev requested a review from Copilot February 3, 2026 22:41

Copilot started reviewing on behalf of evaleev February 3, 2026 22:42 View session

evaleev linked an issue Feb 3, 2026 that may be closed by this pull request

split SeQuant targets #391

Open

Copilot AI reviewed Feb 3, 2026

View reviewed changes

evaleev added this to the 2.1 milestone Feb 3, 2026

Krzmbrzl requested changes Feb 4, 2026

View reviewed changes

		target_compile_definitions(unit_tests-sequant PRIVATE
		SEQUANT_HAS_NUMPY_FOR_VALIDATION=1)

Conversation

evaleev commented Feb 3, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Feb 3, 2026

Choose a reason for hiding this comment

Uh oh!

Krzmbrzl left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

evaleev Feb 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Krzmbrzl Feb 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Krzmbrzl Feb 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Krzmbrzl Feb 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Krzmbrzl Feb 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

evaleev Feb 4, 2026 •

edited

Loading

Krzmbrzl Feb 4, 2026 •

edited

Loading

Krzmbrzl Feb 4, 2026 •

edited

Loading

Krzmbrzl Feb 4, 2026 •

edited

Loading

Krzmbrzl Feb 4, 2026 •

edited

Loading