Add Base Container for Determinant based Wavefunctions#470
Draft
mmoerchen wants to merge 6 commits into
Draft
Conversation
mmoerchen
requested review from
ConradJohnston,
nabbelbabbel,
sobolevnrm and
wavefunction91
as code owners
Contributor
There was a problem hiding this comment.
Pull request overview
This PR introduces an intermediate DeterminantalWavefunctionContainer base class to centralize APIs and serialization behavior for wavefunctions that have an explicit determinant + coefficient expansion, and updates C++/Python bindings plus tests to reflect the new hierarchy and summary/serialization structure.
Changes:
- Add
DeterminantalWavefunctionContainerand move determinant-specific APIs (get_coefficients,get_coefficient,get_active_determinants,size) offWavefunctionContainer. - Refactor
WavefunctionContainerJSON/HDF5 serialization and summary generation to use subclass hooks (_to_json_impl,_to_hdf5_impl,_get_summary_impl) and reuse shared logic. - Update pybind11 bindings and C++/Python tests for the new class structure and updated summary contents.
Reviewed changes
Copilot reviewed 17 out of 17 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| python/tests/test_wavefunction.py | Updates summary assertions to match new summary output (norm line removed). |
| python/src/pybind11/data/wavefunction.cpp | Adds DeterminantalWavefunctionContainer Python binding and moves determinant-specific methods to it; adjusts container inheritance in bindings. |
| cpp/tests/test_wfn_sci.cpp | Updates SCI serialization tests to account for factory-based from_hdf5 return type (now requires downcast). |
| cpp/tests/test_wfn_cas.cpp | Updates CAS serialization tests similarly (factory-based from_hdf5 return type). |
| cpp/tests/test_active_space.cpp | Updates test mock container to inherit determinantal base and implement new serialization hooks. |
| cpp/src/qdk/chemistry/data/wavefunction.cpp | Implements new summary and serialization hook plumbing; gates determinant APIs on DeterminantalWavefunctionContainer. |
| cpp/src/qdk/chemistry/data/wavefunction_containers/sd.cpp | Moves SD serialization to _to_json_impl/_to_hdf5_impl and adds container-specific summary hook. |
| cpp/src/qdk/chemistry/data/wavefunction_containers/sci.cpp | Updates SCI to inherit determinantal base and removes now-redundant coefficient/JSON logic. |
| cpp/src/qdk/chemistry/data/wavefunction_containers/mp2.cpp | Moves MP2 serialization into new hook methods and adds summary hook. |
| cpp/src/qdk/chemistry/data/wavefunction_containers/cc.cpp | Moves CC serialization into new hook methods and adds summary hook. |
| cpp/src/qdk/chemistry/data/wavefunction_containers/cas.cpp | Updates CAS to inherit determinantal base and removes now-redundant coefficient/JSON logic. |
| cpp/include/qdk/chemistry/data/wavefunction.hpp | Introduces DeterminantalWavefunctionContainer and new serialization/summary hooks on WavefunctionContainer. |
| cpp/include/qdk/chemistry/data/wavefunction_containers/sd.hpp | Updates SD container inheritance and declares new hook overrides. |
| cpp/include/qdk/chemistry/data/wavefunction_containers/sci.hpp | Updates SCI container inheritance to determinantal base. |
| cpp/include/qdk/chemistry/data/wavefunction_containers/mp2.hpp | Updates MP2 serialization to use new hook overrides. |
| cpp/include/qdk/chemistry/data/wavefunction_containers/cc.hpp | Updates CC serialization to use new hook overrides and adjusts determinant-API comments. |
| cpp/include/qdk/chemistry/data/wavefunction_containers/cas.hpp | Updates CAS container inheritance to determinantal base. |
Comments suppressed due to low confidence (1)
cpp/src/qdk/chemistry/data/wavefunction_containers/cc.cpp:470
NATIVE_HBOOLexpects anhbool_tbuffer, but this writes the address of a C++bool. On platforms whereboolandhbool_tdiffer in size/representation, this can corrupt the attribute value and break deserialization. Use anhbool_ttemporary (as done in other containers) and write that instead.
// complex flag
bool is_complex = this->is_complex();
H5::Attribute is_complex_attr = group.createAttribute(
"is_complex", H5::PredType::NATIVE_HBOOL, H5::DataSpace(H5S_SCALAR));
is_complex_attr.write(H5::PredType::NATIVE_HBOOL, &is_complex);
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
📊 Coverage Summary
Detailed Coverage ReportsC++ Coverage DetailsPython Coverage DetailsPybind11 Coverage Details |
wavefunction91
requested changes
May 8, 2026
| * @return Reference to vector of CI coefficients | ||
| */ | ||
| const VectorVariant& get_coefficients() const override; | ||
| const VectorVariant& get_coefficients() const; |
Collaborator
There was a problem hiding this comment.
One of the main results of this PR should be that amplitudes should be treated differently than state vector coefficients.
Contributor
Author
There was a problem hiding this comment.
Yes, CC and MP2 now only inherit from the base container and aren't required to provide these methods. They do have CI coefficients implemented, so I kept it.
Should we nix these functions to keep the API cleaner, or rename it to make it more explicit that it involves computation (e.g., compute_ci_expansion())?
| * accessible. Use this type to dispatch on wavefunctions with an explicit | ||
| * determinantal representation. | ||
| */ | ||
| class DeterminantalWavefunctionContainer : public WavefunctionContainer { |
Collaborator
There was a problem hiding this comment.
- This should live in it's own header
- We're going to generalize this concept in a future PR to take non-determinantal statevectors. I'd prefer that this indicates something like a
StateVectorContaineror similar.
Comment on lines
-201
to
-206
| /** | ||
| * @brief Convert container to JSON format | ||
| * @return JSON object containing container data | ||
| */ | ||
| nlohmann::json to_json() const override; | ||
|
|
Collaborator
There was a problem hiding this comment.
Is there a reason to prefer to the _to_json_impl design pattern rather than the previous motif?
Contributor
Author
There was a problem hiding this comment.
This way, each class only has to handle the serialization for its specific data structures instead of implementing the logic of common data (like RDMs, entropies, etc.) as well.
| * @param det Determinant to look up (active space only) | ||
| * @return Scalar coefficient (real or complex) | ||
| */ | ||
| virtual ScalarVariant get_coefficient(const Configuration& det) const = 0; |
Collaborator
There was a problem hiding this comment.
Things to do with Configuration should no longer live in the public Wavefunction API as not all instances support it. This is the wrong place to comment on it, but I don't want to page through this file to find the right line.
| /** | ||
| * @brief Get number of determinants in the expansion | ||
| */ | ||
| virtual size_t size() const = 0; |
Collaborator
There was a problem hiding this comment.
Size has to be polymorphic from the top-level API - currently size downcasts to the determinant container, which will fail for unnecessary reasons.
Contributor
Author
There was a problem hiding this comment.
Okay, but what should size() return for a non-determinantal wavefunction?
I move the size method to the StateVectorContainer, which is why I downcast.
Comment on lines
-165
to
+163
| .def("get_orbital_occupations", | ||
| &WavefunctionContainer::get_active_orbital_occupations, | ||
| "Get orbital occupations for active orbitals") | ||
| .def("get_active_orbital_occupations", | ||
| &WavefunctionContainer::get_total_orbital_occupations, | ||
| "Get orbital occupations for all orbitals") | ||
| .def("get_active_orbital_occupations", | ||
| &WavefunctionContainer::get_active_orbital_occupations, | ||
| "Get orbital occupations for active orbitals only") | ||
| .def("get_total_orbital_occupations", | ||
| &WavefunctionContainer::get_total_orbital_occupations, | ||
| "Get orbital occupations for all orbitals") |
Collaborator
There was a problem hiding this comment.
Why change this convention?
Contributor
Author
There was a problem hiding this comment.
get_active_orbital_occupations was defined twice (with active and total occupations), and get_orbital_occupations didn't follow the active/total pattern we usually use
| QDK_LOG_TRACE_ENTERING(); | ||
|
|
||
| // Store restrictedness flag | ||
| bool is_restricted = get_orbitals()->is_restricted(); |
Collaborator
There was a problem hiding this comment.
This should be guarded on a null check for the orbitals instance
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.