Add auxiliary basis set info to BasisSet#441
Open
rainli323 wants to merge 38 commits into
Open
Conversation
📊 Coverage Summary
Detailed Coverage ReportsC++ Coverage DetailsPython Coverage DetailsPybind11 Coverage Details |
rainli323
requested review from
ConradJohnston,
nabbelbabbel and
wavefunction91
as code owners
Contributor
There was a problem hiding this comment.
Pull request overview
This PR extends BasisSet to carry primary, ECP, and auxiliary (density-fitting) basis information across C++, Python bindings, serialization, and docs, while removing/avoiding constructors that could create invalid basis sets.
Changes:
- Adds auxiliary-basis support (constructors, accessors, serialization, docs/examples) alongside existing primary/ECP data.
- Updates Python/pybind bindings to support the new constructor shapes (including an
__init__dispatcher forpy::smart_holder). - Updates Python and C++ tests to cover auxiliary basis behavior and revised ECP construction requirements.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| python/tests/test_pyscf_plugin.py | Updates PySCF plugin tests for new BasisSet ECP constructor shape. |
| python/tests/test_basis_set.py | Adds/updates Python tests for auxiliary basis accessors + serialization + constructors. |
| python/src/qdk_chemistry/plugins/pyscf/conversion.py | Adjusts PySCF→QDK BasisSet creation logic for revised ECP constructor requirements. |
| python/src/pybind11/data/basis_set.cpp | Reworks Python BasisSet constructor binding and exposes auxiliary-basis APIs + overloads. |
| docs/source/user/comprehensive/data/basis_set.rst | Documents ECP + auxiliary basis capabilities and serialization fields. |
| docs/source/_static/examples/python/basis_set.py | Adds Python examples for ECP and auxiliary basis usage. |
| docs/source/_static/examples/cpp/basis_set.cpp | Adds C++ examples for ECP and auxiliary basis usage. |
| cpp/tests/test_basis_set.cpp | Updates/expands C++ test coverage for aux basis and revised ECP constructors. |
| cpp/src/qdk/chemistry/data/basis_set.cpp | Implements auxiliary basis storage/access, new constructors, and serialization support. |
| cpp/include/qdk/chemistry/data/basis_set.hpp | Declares new constructors/static factories and auxiliary-basis accessors. |
Comments suppressed due to low confidence (1)
cpp/src/qdk/chemistry/data/basis_set.cpp:380
- The ECP-shells constructor does not validate that
nameis non-empty, while the(name, shells, structure, ...)constructor explicitly throws whennameis empty. This can lead to inconsistent behavior (e.g.,BasisSet("", shells, ecp_shells, ecp_electrons, structure)succeeds until later code relies on a non-empty name). Add the sameif (_name.empty()) throw ...guard to this and the other constructors that acceptname.
: _name(name),
_atomic_orbital_type(atomic_orbital_type),
_structure(structure),
_ecp_name("none"),
_ecp_electrons(ecp_electrons) {
QDK_LOG_TRACE_ENTERING();
if (!structure) {
throw std::invalid_argument("Structure shared_ptr cannot be nullptr");
}
💡 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 10 out of 10 changed files in this pull request and generated 5 comments.
Comments suppressed due to low confidence (1)
cpp/src/qdk/chemistry/data/basis_set.cpp:381
- This constructor does not validate that
nameis non-empty, unlike the(name, shells, structure, ...)constructor which throws on an empty basis name. As a result,BasisSet("", shells, ecp_shells, ecp_electrons, structure)can be created with an invalid empty name. Consider adding the sameif (_name.empty()) throw ...;check here (and in the other structure-taking constructors) for consistent validation.
BasisSet::BasisSet(const std::string& name, const std::vector<Shell>& shells,
const std::vector<Shell>& ecp_shells,
const std::vector<size_t>& ecp_electrons,
std::shared_ptr<Structure> structure,
AOType atomic_orbital_type)
: _name(name),
_atomic_orbital_type(atomic_orbital_type),
_structure(structure),
_ecp_name("none"),
_ecp_electrons(ecp_electrons) {
QDK_LOG_TRACE_ENTERING();
if (!structure) {
throw std::invalid_argument("Structure shared_ptr cannot be nullptr");
}
💡 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 10 out of 10 changed files in this pull request and generated 5 comments.
Comments suppressed due to low confidence (2)
cpp/src/qdk/chemistry/data/basis_set.cpp:2514
- The same stale fallback exists in HDF5 deserialization: when
ecp_shellsare present butecp_electronsare missing, this now resolves to the auxiliary-shell constructor that was added in this PR. Older HDF5 files with ECP shells but no electron-count dataset will therefore stop loading and surface a misleading auxiliary-shell error.
} else if (!ecp_shells.empty()) {
if (!ecp_name.empty() && !ecp_electrons.empty()) {
basis_set = std::make_shared<BasisSet>(
name, shells, ecp_name, ecp_shells, ecp_electrons, *structure,
atomic_orbital_type);
} else {
basis_set = std::make_shared<BasisSet>(
name, shells, ecp_shells, *structure, atomic_orbital_type);
}
cpp/src/qdk/chemistry/data/basis_set.cpp:2935
- This fallback branch still calls the old 4-argument
(name, shells, ..., structure)constructor when JSON containsecp_shellsbut noecp_electrons. After this PR that overload is the auxiliary-shell constructor, so legacy payloads will now fail with the auxiliary-shell radial-powers error instead of deserializing or producing a clear compatibility error.
} else if (!ecp_shells.empty()) {
if (!ecp_name.empty() && !ecp_electrons.empty()) {
basis_set = std::make_shared<BasisSet>(
name, shells, ecp_name, ecp_shells, ecp_electrons, *structure,
atomic_orbital_type);
} else {
basis_set = std::make_shared<BasisSet>(
name, shells, ecp_shells, *structure, atomic_orbital_type);
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <copilot@github.com>
Co-authored-by: Copilot <copilot@github.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 10 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (2)
cpp/src/qdk/chemistry/data/basis_set.cpp:2948
- If JSON input contains
ecp_shellsbut omits the new ECP metadata, this branch now calls the 4-argument(name, shells, ..., structure)overload after the old ECP-only constructor was removed. That overload is the auxiliary-basis constructor, so legacy ECP JSON is misrouted here and either gets misclassified or fails with the auxiliary-shell radial-powers error instead of a clear "missing ecp_electrons" failure.
name, shells, ecp_name, ecp_shells, ecp_electrons, *structure,
atomic_orbital_type);
} else {
basis_set = std::make_shared<BasisSet>(
name, shells, ecp_shells, *structure, atomic_orbital_type);
}
} else {
basis_set = std::make_shared<BasisSet>(name, shells, *structure,
cpp/src/qdk/chemistry/data/basis_set.cpp:2958
- If JSON contains
aux_shells/aux_namebut nostructure, this fallback ignores the auxiliary block and returns a primary-onlyBasisSet. Since auxiliary shells are only representable with a structure, the loader should fail here rather than silently discarding serialized data.
throw std::runtime_error(
"Cannot create BasisSet with ECP shells but without structure");
}
basis_set = std::make_shared<BasisSet>(name, shells, atomic_orbital_type);
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <copilot@github.com>
Co-authored-by: Copilot <copilot@github.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 10 changed files in this pull request and generated 6 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <copilot@github.com>
Co-authored-by: Copilot <copilot@github.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 10 changed files in this pull request and generated 7 comments.
Comments suppressed due to low confidence (1)
docs/source/user/comprehensive/data/basis_set.rst:1
- This section still says there are 'three methods' immediately before documenting an additional auxiliary-loading entry point, so the count is now inaccurate.
BasisSet
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <copilot@github.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 10 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+1743
to
+1745
| bool ecp_consistency = (has_ecp_electrons() == has_ecp_shells()); | ||
|
|
||
| return has_shells && _is_consistent_with_structure() && ecp_consistency; |
Co-authored-by: Copilot <copilot@github.com>
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.
BasisSet class now include primary, auxiliary, and ECP basis information.
BasisSet can be constructed with information of
Several old BasisSet constructors have been removed, including those that: