Open
Conversation
…microsoft/qdk-chemistry into feature_RL_Hamiltonian_refactor
… of hamiltonian containers
📊 Coverage Summary
Detailed Coverage ReportsC++ Coverage DetailsPython Coverage DetailsPybind11 Coverage Details |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR adds density-fitted (RI/DF) Hamiltonian support to QDK/Chemistry by introducing a new Hamiltonian container that stores 3-center integrals and a corresponding Hamiltonian constructor that requires an auxiliary basis, plus updates to bindings, docs, and tests.
Changes:
- Added
DensityFittedHamiltonianContainerwith lazy on-demand 4-center integral materialization and JSON/HDF5/FCIDUMP support. - Added
microsoft::DensityFittedHamiltonianConstructorand extended the Hamiltonian-constructor API to optionally accept an auxiliary basis. - Updated Python bindings/tests/docs to expose and validate the new container/constructor, including an O2 DF-MP2 integration test.
Reviewed changes
Copilot reviewed 29 out of 30 changed files in this pull request and generated 12 comments.
Show a summary per file
| File | Description |
|---|---|
| python/tests/test_helpers.py | Extends test Hamiltonian factory to create canonical vs density-fitted containers. |
| python/tests/test_hamiltonian.py | Parameterizes Hamiltonian tests across container types; adds DF-specific and factory integration tests. |
| python/src/qdk_chemistry/data/init.py | Re-exports DensityFittedHamiltonianContainer in the Python data package. |
| python/src/qdk_chemistry/algorithms/hamiltonian_constructor.py | Re-exports QdkDensityFittedHamiltonianConstructor. |
| python/src/qdk_chemistry/algorithms/init.py | Exposes QdkDensityFittedHamiltonianConstructor at package level. |
| python/src/pybind11/data/hamiltonian.cpp | Binds DensityFittedHamiltonianContainer and updates container docstrings/examples. |
| python/src/pybind11/algorithms/hamiltonian.cpp | Adds run(orbitals, aux_basis) overload and binds QdkDensityFittedHamiltonianConstructor. |
| docs/source/user/comprehensive/data/hamiltonian.rst | Documents container types and DF usage. |
| docs/source/user/comprehensive/algorithms/hamiltonian_constructor.rst | Documents the DF constructor and its factory name/usage. |
| docs/source/glossary.rst | Adds DF/RI glossary entries. |
| cpp/src/qdk/chemistry/data/hamiltonian_containers/density_fitted.cpp | Implements DF container, lazy 4-center cache, and serialization. |
| cpp/src/qdk/chemistry/data/hamiltonian_containers/canonical_four_center.cpp | Removes FCIDUMP implementation now moved to base container. |
| cpp/src/qdk/chemistry/data/hamiltonian.cpp | Adds DF container deserialization and moves FCIDUMP writing to base container; strengthens validation. |
| cpp/src/qdk/chemistry/data/CMakeLists.txt | Adds DF container source to build. |
| cpp/src/qdk/chemistry/algorithms/microsoft/mp2.cpp | Uses active-space orbital energies for MP2 computations. |
| cpp/src/qdk/chemistry/algorithms/microsoft/hamiltonian_util.hpp/.cpp | Introduces shared helper utilities (active-space validation; 3-center transforms; J/K builds). |
| cpp/src/qdk/chemistry/algorithms/microsoft/hamiltonian.hpp/.cpp | Updates constructor signature to accept optional aux basis and rejects aux basis for non-DF constructor. |
| cpp/src/qdk/chemistry/algorithms/microsoft/density_fitted_hamiltonian.hpp/.cpp | Adds DF Hamiltonian constructor implementation. |
| cpp/src/qdk/chemistry/algorithms/microsoft/cholesky_hamiltonian.hpp/.cpp | Updates signature to accept optional aux basis and rejects aux basis for Cholesky constructor; refactors helpers. |
| cpp/src/qdk/chemistry/algorithms/microsoft/CMakeLists.txt | Adds DF constructor + shared util sources to build. |
| cpp/src/qdk/chemistry/algorithms/hamiltonian.cpp | Registers DF Hamiltonian constructor factory instance. |
| cpp/include/qdk/chemistry/data/hamiltonian_containers/density_fitted.hpp | Adds DF container public C++ API. |
| cpp/include/qdk/chemistry/data/hamiltonian_containers/canonical_four_center.hpp | Updates canonical container docs and removes FCIDUMP override. |
| cpp/include/qdk/chemistry/data/hamiltonian.hpp | Makes to_fcidump_file non-pure virtual (base implementation). |
| cpp/include/qdk/chemistry/algorithms/hamiltonian.hpp | Extends constructor API to accept OptionalAuxBasis and adds run(orbitals) convenience overload. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR adds first-class density-fitting (RI/DF) support for Hamiltonian construction and storage across the C++ core, Python bindings, docs, and tests, enabling three-center integral storage with lazy four-center materialization for post-SCF workflows (e.g., DF-MP2).
Changes:
- Introduces
DensityFittedHamiltonianContainer(C++ + pybind) with three-center integral storage, lazy four-center cache, and JSON/HDF5/FCIDUMP support. - Adds
microsoft::DensityFittedHamiltonianConstructorand extendsHamiltonianConstructor.run()to support an optional auxiliary basis parameter (while keeping a 1-arg convenience overload). - Expands/parameterizes Python tests and documentation to cover the new container type and density-fitted construction.
Reviewed changes
Copilot reviewed 29 out of 30 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| python/tests/test_helpers.py | Extends test helper to construct Hamiltonians with either canonical or density-fitted containers. |
| python/tests/test_hamiltonian.py | Parameterizes Hamiltonian tests across container types and adds DF constructor integration test. |
| python/src/qdk_chemistry/data/init.py | Re-exports DensityFittedHamiltonianContainer in the public Python data API. |
| python/src/qdk_chemistry/algorithms/hamiltonian_constructor.py | Re-exports the new density-fitted constructor binding. |
| python/src/qdk_chemistry/algorithms/init.py | Exposes QdkDensityFittedHamiltonianConstructor in the algorithms package surface. |
| python/src/pybind11/data/hamiltonian.cpp | Binds DensityFittedHamiltonianContainer to Python and updates integral-notation docs. |
| python/src/pybind11/algorithms/hamiltonian.cpp | Adds run(orbitals, aux_basis) overload and binds QdkDensityFittedHamiltonianConstructor. |
| docs/source/user/comprehensive/data/hamiltonian.rst | Documents Hamiltonian container types, including density-fitted container usage. |
| docs/source/user/comprehensive/algorithms/hamiltonian_constructor.rst | Documents the DF Hamiltonian constructor factory implementation and aux-basis requirement. |
| docs/source/glossary.rst | Adds glossary entries for DF and RI. |
| cpp/src/qdk/chemistry/data/hamiltonian_containers/density_fitted.cpp | Implements DF container logic, lazy four-center construction, and JSON/HDF5 serialization. |
| cpp/src/qdk/chemistry/data/hamiltonian_containers/canonical_four_center.cpp | Removes FCIDUMP writing implementation (moved to base container implementation). |
| cpp/src/qdk/chemistry/data/hamiltonian.cpp | Routes container deserialization for DF and moves FCIDUMP writing into HamiltonianContainer. |
| cpp/src/qdk/chemistry/data/CMakeLists.txt | Adds DF container source to the data target. |
| cpp/src/qdk/chemistry/algorithms/microsoft/mp2.cpp | Fixes MP2 to use active-space orbital energies (consistent with active-space Hamiltonians). |
| cpp/src/qdk/chemistry/algorithms/microsoft/hamiltonian_util.hpp | Introduces shared helpers for active-index validation and three-center transformations/J/K builds. |
| cpp/src/qdk/chemistry/algorithms/microsoft/hamiltonian_util.cpp | Implements shared helper functions used by Cholesky and DF paths. |
| cpp/src/qdk/chemistry/algorithms/microsoft/hamiltonian.hpp | Updates four-center Hamiltonian constructor to the new _run_impl(orbitals, aux_basis) signature. |
| cpp/src/qdk/chemistry/algorithms/microsoft/hamiltonian.cpp | Enforces aux-basis is not accepted by the four-center constructor. |
| cpp/src/qdk/chemistry/algorithms/microsoft/density_fitted_hamiltonian.hpp | Declares the DF Hamiltonian constructor and its settings. |
| cpp/src/qdk/chemistry/algorithms/microsoft/density_fitted_hamiltonian.cpp | Implements DF Hamiltonian construction using an auxiliary basis and three-center machinery. |
| cpp/src/qdk/chemistry/algorithms/microsoft/cholesky_hamiltonian.hpp | Updates Cholesky constructor signature and reuses shared three-center utilities. |
| cpp/src/qdk/chemistry/algorithms/microsoft/cholesky_hamiltonian.cpp | Refactors Cholesky constructor to shared utilities and rejects aux-basis. |
| cpp/src/qdk/chemistry/algorithms/microsoft/CMakeLists.txt | Adds DF constructor + shared utility sources to the microsoft algorithms target. |
| cpp/src/qdk/chemistry/algorithms/hamiltonian.cpp | Registers the DF constructor in the factory. |
| cpp/include/qdk/chemistry/data/hamiltonian_containers/density_fitted.hpp | Adds the public DF container header/API. |
| cpp/include/qdk/chemistry/data/hamiltonian_containers/canonical_four_center.hpp | Removes per-container FCIDUMP API and updates doc comments. |
| cpp/include/qdk/chemistry/data/hamiltonian.hpp | Makes HamiltonianContainer::to_fcidump_file a concrete base implementation. |
| cpp/include/qdk/chemistry/algorithms/hamiltonian.hpp | Extends constructor API to accept optional auxiliary basis (variant of BasisSet ptr or name). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR introduces a new density-fitted Hamiltonian representation and constructor path, enabling memory-efficient storage of MO-space ERIs via three-center integrals with lazy on-demand reconstruction of four-center integrals. It also extends the Hamiltonian-constructor API to accept an optional auxiliary basis and wires the new implementation through C++/Python bindings, documentation, and tests.
Changes:
- Added
DensityFittedHamiltonianContainer(C++ + pybind) with three-center integral storage and lazy four-center cache generation. - Added
qdk_density_fitted_hamiltonianconstructor implementation and extendedHamiltonianConstructor::run/_run_implto accept an optional auxiliary basis. - Updated Python exports, docs, and tests (parameterized container tests + DF MP2 integration test).
Reviewed changes
Copilot reviewed 28 out of 29 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| python/tests/test_helpers.py | Extends test Hamiltonian factory to create canonical vs density-fitted containers. |
| python/tests/test_hamiltonian.py | Parameterizes Hamiltonian tests across container types and adds DF constructor integration test. |
| python/src/qdk_chemistry/data/init.py | Exposes DensityFittedHamiltonianContainer in the public Python data API. |
| python/src/qdk_chemistry/algorithms/hamiltonian_constructor.py | Re-exports QdkDensityFittedHamiltonianConstructor from core bindings. |
| python/src/qdk_chemistry/algorithms/init.py | Adds DF constructor to algorithm package exports. |
| python/src/pybind11/data/hamiltonian.cpp | Binds DensityFittedHamiltonianContainer and updates docstrings/indexing notes. |
| python/src/pybind11/algorithms/hamiltonian.cpp | Updates HamiltonianConstructor.run binding to accept optional aux basis and binds QdkDensityFittedHamiltonianConstructor. |
| docs/source/user/comprehensive/data/hamiltonian.rst | Documents container types and density-fitted container usage. |
| docs/source/user/comprehensive/algorithms/hamiltonian_constructor.rst | Documents the new qdk_density_fitted_hamiltonian implementation and aux-basis requirement. |
| docs/source/glossary.rst | Adds DF/RI glossary entries. |
| cpp/src/qdk/chemistry/data/hamiltonian_containers/density_fitted.cpp | Implements the new DF container with lazy four-center cache + JSON/HDF5 I/O. |
| cpp/src/qdk/chemistry/data/hamiltonian_containers/canonical_four_center.cpp | Removes FCIDUMP logic (moved to base container). |
| cpp/src/qdk/chemistry/data/hamiltonian.cpp | Adds DF container dispatch + moves FCIDUMP writer to HamiltonianContainer. |
| cpp/src/qdk/chemistry/data/CMakeLists.txt | Adds DF container source to build. |
| cpp/src/qdk/chemistry/algorithms/microsoft/hamiltonian_util.hpp | Adds shared helpers for active-index validation and three-center transformations/J/K builds. |
| cpp/src/qdk/chemistry/algorithms/microsoft/hamiltonian_util.cpp | Implements the new shared Hamiltonian utilities. |
| cpp/src/qdk/chemistry/algorithms/microsoft/hamiltonian.hpp | Updates four-center constructor signature to accept optional aux basis (and reject it at runtime). |
| cpp/src/qdk/chemistry/algorithms/microsoft/hamiltonian.cpp | Implements aux-basis rejection for the four-center constructor. |
| cpp/src/qdk/chemistry/algorithms/microsoft/density_fitted_hamiltonian.hpp | Declares DF constructor implementation and settings. |
| cpp/src/qdk/chemistry/algorithms/microsoft/density_fitted_hamiltonian.cpp | Implements DF Hamiltonian construction using an auxiliary basis and three-center integrals. |
| cpp/src/qdk/chemistry/algorithms/microsoft/cholesky_hamiltonian.hpp | Updates Cholesky constructor signature and reuses shared utilities. |
| cpp/src/qdk/chemistry/algorithms/microsoft/cholesky_hamiltonian.cpp | Updates Cholesky constructor to accept (and reject) aux basis and reuse shared utilities. |
| cpp/src/qdk/chemistry/algorithms/microsoft/CMakeLists.txt | Adds DF constructor + shared utility sources to build. |
| cpp/src/qdk/chemistry/algorithms/hamiltonian.cpp | Registers qdk_density_fitted_hamiltonian in the factory. |
| cpp/include/qdk/chemistry/data/hamiltonian_containers/density_fitted.hpp | Declares the DF container public C++ API. |
| cpp/include/qdk/chemistry/data/hamiltonian_containers/canonical_four_center.hpp | Removes FCIDUMP override and updates docs/comments. |
| cpp/include/qdk/chemistry/data/hamiltonian.hpp | Makes HamiltonianContainer::to_fcidump_file non-pure virtual (base implementation). |
| cpp/include/qdk/chemistry/algorithms/hamiltonian.hpp | Adds OptionalAuxBasis and updates HamiltonianConstructor API to accept it. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
mmoerchen
suggested changes
Apr 1, 2026
| @@ -0,0 +1,198 @@ | |||
| // Copyright (c) Microsoft Corporation. All rights reserved. | |||
Contributor
There was a problem hiding this comment.
The content of this file can be moved into the hamiltonian.cpp and .hpp
| /** | ||
| * @class CanonicalFourCenterHamiltonianContainer | ||
| * @brief Contains a molecular Hamiltonian using canonical four center | ||
| * integrals, implemented as a subclass of HamiltonianContainer. |
Contributor
There was a problem hiding this comment.
What was the reason for changing the class docstring here?
Contributor
Author
There was a problem hiding this comment.
Because a previous reviewer does not think the implementation detail is relevant to users.
| py::arg("filename"), py::arg("nalpha"), py::arg("nbeta")); | ||
|
|
||
| // ============================================================================ | ||
| // DensityFittedHamiltonianContainer - Density-fitted integral storage |
Contributor
There was a problem hiding this comment.
Why is this different than the cholesky hamiltonian? Could this container work for both methods?
Contributor
Author
There was a problem hiding this comment.
They would need to be merged in the next PR, not this one.
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 28 out of 29 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 32 out of 33 changed files in this pull request and generated 6 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
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.
Density fitted hamiltonian is provided for post SCF calculations.
DensityFittedHamiltonianContaineris created for density fitted Hamiltonian in the MO space. The three center integrals are stored in memory, and unless get_two_body_integrals() are called, the 4 center integrals are never calculated or stored. If a user wants to obtain the three-center integrals,Hamiltonian.get_container().get_three_center_integrals()must be called, otherwiseHamiltonian.get_container().get_two_body_integrals()andHamiltonian.get_two_body_integrals()will give the constructed 4 center integrals. The 3 center integrals are stored in a matrix of sizen_aux * n_geminal, wheren_geminalisn_orb * n_orb, andn_auxis the number of auxiliary basis functions.microsoft::DensityFittedHamiltonianConstructoris created as a derived class ofalgorithm::HamiltonanConstructor. It can be created byHamiltonianConstructorFactory, then uponrun()createsHamiltoniancorresponding toDensityFittedHamiltonianContainer. Therun()function requires orbital and auxiliary basis set as parameters. Therun()function does not break the API for other Hamiltonian Constructors.ctestfor various Hamiltonian Containers (data) are done withTEST_P, which runs the same tests for each container. The integrated test for constructing the DF Hamiltonian is DFMP2, where normal MP2 used integrals reconstructed from DF Hamiltonian to obtain correlation energy, and the result is compared with Psi4.