-
Notifications
You must be signed in to change notification settings - Fork 8
[export] minimal numpy support #429
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
Conversation
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 adds minimal NumPy and PyTorch export support to SeQuant through a new PythonEinsumGenerator that generates Python code using einsum notation for tensor contractions. The implementation includes comprehensive validation tests that execute generated Python code and verify correctness by comparing results computed in C++ (Eigen) with Python (NumPy).
Key changes:
- New
PythonEinsumGeneratorfor generating Python/NumPy/PyTorch code with einsum operations - Custom NumPy I/O utilities for reading/writing Eigen tensors in .npy format with proper layout handling
- Refactored test context setup by extracting
to_export_context()to a shared header file - Enhanced expression operator overloads to support scalar multiplication with
ExprPtr
Reviewed Changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| SeQuant/core/export/python_einsum.hpp | Implements PythonEinsumGenerator and PythonEinsumGeneratorContext for generating Python einsum code with NumPy/PyTorch backend support |
| tests/unit/test_export_python.cpp | Comprehensive validation tests including NumPy I/O utilities and end-to-end Python execution tests |
| tests/unit/test_export.hpp | Shared test context setup extracted from test_export.cpp for reuse across test files |
| tests/unit/test_export.cpp | Updated to use shared context from test_export.hpp and added PythonEinsumGenerator to test suite |
| tests/unit/CMakeLists.txt | Added Python/NumPy/PyTorch detection logic with conditional compilation for validation tests |
| SeQuant/core/expressions/constant.hpp | Improved template constraints for Constant constructor to prevent ambiguous conversions |
| SeQuant/core/expressions/expr_operators.hpp | Added operator overloads for scalar * ExprPtr and ExprPtr * scalar |
| tests/unit/export_tests/binary_contract.export_test | Added expected Python einsum output for binary contraction test case |
| .github/workflows/cmake.yml | Updated CI to install numpy package on macOS and Ubuntu for validation tests |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
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
Copilot reviewed 11 out of 11 changed files in this pull request and generated 7 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
54556c0 to
1f3c2e6
Compare
Co-authored-by: Copilot <[email protected]>
Implement proper POSIX shell escaping for script path using single quotes. This prevents command injection when the path contains special shell metacharacters like backticks, dollar signs, or spaces. Co-authored-by: evaleev <[email protected]>
Co-authored-by: Copilot <[email protected]>
cbe70f5 to
b11a07b
Compare
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
Copilot reviewed 12 out of 13 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Krzmbrzl
left a comment
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.
More thorough review will come in the coming days
Krzmbrzl
left a comment
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.
The test cases need to be extended to cover a more diverse set of exports. At the very least, I believe we need a ternary contraction, a sum of expressions and an example using a named section (function).
| bool requires_named_sections() const override { return false; } | ||
|
|
||
| DeclarationScope index_declaration_scope() const override { | ||
| return DeclarationScope::Global; |
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.
I believe Section would be more appropriate. Index declarations can be (ab)used to add parameters representing index dimensions to function (aka: "named section") signatures. Otherwise, the variables holding the index dimensions would have to be global.
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.
In python all declare() methods are no-ops ... don't have a good feel for this in absence of use cases. Julia seems to use Global ... please advise
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.
If you want to make the generated code rely on global variables (as it currently does), whatever this function returns is insignificant. My suggestion was to make use of index declarations in order to generate functions that take the index dimensions as parameters:
def bla(n_occ, n_virt):
passfor which we would need DeclarationScope::Section
…ce code and numerics
added the cited tests via 142bb86 663b35c dc3cf69 ... ideally need to validate everything that ITF exporter. Unfortunately we only have codegen tests for non-Python backends, so Python needs to have the most extensive testset, with matching numerical tests |
|
@Krzmbrzl is the person who added Julia codegen still around? Would it be possible to implement support for numerical testing of Julia code along the lines of test_export_python.cpp? |
I think we could get a long way by having Python do integration tests in addition to unit tests by e.g. generating a CCSD residual code and then evaluating that for some non-zero amplitudes and then comparing to a pre-computed (and validated) result. That should contain most of the individual elements that the unit tests for current exporters test.
Unfortunately, no. He was only there for a 3 month summer internship. If I find the time, I might be able to add (some of) that myself but there are a couple other things on my plate rn that have a higher priority for me 👀 |
For the export paper (or whatever paper includes it) might be useful. I'll focus on unit tests first.
understood. AI bots are pretty good at things like this, especially since we already have the template for python (with Eigen::Tensor as the source of truth). It's just matter of getting CI configured to pull Julia, have CMake harness detect it, and then generating the numerical tests themselves. |
…of the julia counterpart (base -> {numpy,pytorch} variants}
…nning of the export
| }; | ||
|
|
||
| /// Generator for NumPy einsum | ||
| class NumPyEinsumGenerator |
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.
All generators should be templated based on their context type. This is needed in order to make inheritance easier. The reason being that we want don't want to require the correct context class to be used with the generator (see also https://stackoverflow.com/q/18671062).
That is, we don't want to allow NumPyEinsumGenerator to be fed a PythonEinsumGeneratorContext (which could also be the PyTorch one). This is already given thanks to CRTP on the base class. However, it would be nice to just keep the option of inheriting from NumPyEinsumGenerator as well (which requires it to have a CRTP for the context class).
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.
For "final" generators the context<->generator is 1<->1, removing ability to change context limits customization but until we run into the need to override the default I'll leave as is.
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.
Yes but the problem is that handling of classes vs. template classes can be different in certain contexts. Hence, I would strongly suggest we make it a template as all other generator classes to ensure we don't have to refactor call-sides should we ever encounter the need.
a3294de to
7901dcc
Compare
|
many other issues remain (e.g. how not to hardwire to fp64, etc.) but good enough for the first version @Krzmbrzl |
No description provided.