-
Notifications
You must be signed in to change notification settings - Fork 8
Separate optimize #484
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Separate optimize #484
Conversation
b2c788c to
9e3e987
Compare
9e3e987 to
23718ba
Compare
…leClang 16 (and probably earlier). AppleClang 17 (used in CI) and recent LLVM Clang are OK.
…s a linker. Homebrew LLVM Clang is not usable for linking without extra flags due to the linker defaulting to the system libc++. This check provides the workaround by adding extra linker flags if the given clang is not usable for linking.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR refactors SeQuant’s optimization layer by moving optimize-related headers/sources out of core/eval/optimize/* into a dedicated core/optimize/* area, and updates downstream includes/usages accordingly. It also adds CMake-time checks to catch AppleClang template support issues and to mitigate libc++ header/library mismatches for non-Apple Clang toolchains.
Changes:
- Relocated optimization APIs (optimize entrypoints, fusion, sum reordering, CSE) into
SeQuant/core/optimize/*and updated call sites/includes/namespaces (sequant::opt). - Updated unit/integration tests and external-interface utilities to use the new optimize include paths and namespaces.
- Added CMake compiler/toolchain checks (AppleClang minimum version, libc++ header/library mismatch detection) and ensured the new CMake module is available to consumers via the package config.
Reviewed changes
Copilot reviewed 19 out of 20 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| utilities/external-interface/processing.cpp | Switch optimize include to new core/optimize location. |
| utilities/external-interface/external_interface.cpp | Update CSE include path and qualify CSE call with opt::. |
| tests/unit/test_optimize.cpp | Update includes and namespaces for optimize + CSE. |
| tests/unit/test_fusion.cpp | Update fusion include path to new optimize module. |
| tests/unit/test_export.cpp | Update optimize include path. |
| tests/unit/test_eval_tapp.cpp | Update optimize include path. |
| tests/unit/test_eval_btas.cpp | Update optimize include path. |
| tests/integration/eval/calc_info.hpp | Stop depending on removed opt::tail_factor by adding local tail_factor; update optimize include. |
| cmake/sequant-config.cmake.in | Run libc++ mismatch check in the installed package config. |
| cmake/modules/CheckCXXFeatures.cmake | Add check_libcxx_linker_mismatch() implementation. |
| SeQuant/domain/mbpt/spin.cpp | Remove now-unneeded optimize include. |
| SeQuant/core/optimize/sum.hpp | New public API for sum clustering/reordering. |
| SeQuant/core/optimize/sum.cpp | Move sum clustering/reordering implementation into optimize module. |
| SeQuant/core/optimize/single_term.hpp | Restructure internals into sequant::opt::detail and trim exposed declarations. |
| SeQuant/core/optimize/optimize.hpp | New public optimize entrypoint header. |
| SeQuant/core/optimize/optimize.cpp | New optimize entrypoint implementation (previously under eval/optimize). |
| SeQuant/core/optimize/fusion.hpp | New public fusion header in optimize module. |
| SeQuant/core/optimize/fusion.cpp | Update include path to new fusion header. |
| SeQuant/core/optimize/common_subexpression_elimination.hpp | Move CSE into sequant::opt namespace. |
| CMakeLists.txt | Register new optimize sources; add AppleClang version gate; install/copy CheckCXXFeatures for package config usage. |
Comments suppressed due to low confidence (2)
SeQuant/core/optimize/sum.cpp:5
SeQuant/core/optimize/sum.cppnow usesstd::stackand multiple Range-v3 utilities (ranges::front,ranges::views::keys,ranges::to,ranges::views::reverse, etc.) but the corresponding standard/Range-v3 headers were removed from this translation unit. As-is, this file won’t compile unless those headers are included transitively (which they are not via the current includes). Add the required<stack>and the specific Range-v3 headers needed forviews/to/keys/reverse(or include an umbrella Range-v3 header used elsewhere in the project).
SeQuant/core/optimize/single_term.hpp:14single_term.hppusesstd::floor(inbiparts) andstd::invoke(inops_count), but the standard headers that guarantee these declarations (<cmath>and<functional>) were removed from this header. Relying on transitive includes is brittle and can break builds depending on include order. Add the missing standard includes (or avoid these APIs) so this header is self-contained.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…ning off the assertions
b0a79be to
3cfd897
Compare
…ibc++ linker issues are checked always + moved CheckCXXFeatures to vg/kit-cmake
No description provided.