CABS, rework build_cabs_space #2982
Conversation
|
The fix to one-electron integrals should be its own pull request. It has nothing to do with the other changes, which are pretty extensive by themselves to review. |
EricaCMitchell
force-pushed
the
CABS
branch
3 times, most recently
from
484314c to
be7ab1b
Compare
| key=lambda shell: shell.l) | ||
|
|
||
| # Molecule and parser prepped, call the constructor | ||
| mol.set_basis_all_atoms(name, "CABS") |
There was a problem hiding this comment.
This isn't allowing basis { ... } style input, but that's fine for now.
Co-authored-by: Lori A. Burns <lori.burns@gmail.com>
Co-authored-by: Lori A. Burns <lori.burns@gmail.com>
|
@konpat, do you think this PR would benefit from you looking it over? I know you have some F12 work in mind. |
|
Yup, @JonathonMisiewicz, I do have this PR on my radar and it's overall a super useful contribution! Unfortunately, I'm very short on time for the next couple weeks and it might still be a while until I properly test it out. |
tests/pytests/test_orbitalspace.py
Outdated
| S_mo = np.linalg.multi_dot([C1.T, S_ao, C2]) | ||
|
|
||
| if o1 != o2: | ||
| np.testing.assert_allclose(np.dot(S_mo.T, S_mo), np.zeros((C2.shape[1], C2.shape[1])), rtol=1e-05, atol=1e-07) |
There was a problem hiding this comment.
Does this test really need to verify that STS is identity or zero? I think we just need to check the matrix S itself for orthonormality (or orthogonality between different spaces):
if o1 != o2:
np.testing.assert_allclose(S_mo, np.zeros((C1.shape[1], C2.shape[1])), rtol=1e-05, atol=1e-07)
else:
np.testing.assert_allclose(S_mo, np.eye(C1.shape[1]), rtol=1e-05, atol=1e-07)
There was a problem hiding this comment.
Another (very picky) issue: shouldn't we prefer to check if o1 and o2 are approximately equal, to within some numerical threshold?
There was a problem hiding this comment.
@konpat You're right! I've fixed the test to just look at the S_mo matrix.
@JonathonMisiewicz The two spaces are going to be unequal just by construction with the CABS space consisting of the basis functions of the OBS. What I'm checking for here is that we are fulfilling the condition that S12 C = 0, as described in the paper by Valeev. I guess I do check more cases than is necessary in the test though.
tests/pytests/test_orbitalspace.py
Outdated
| S_ao = np.array(mints.ao_overlap(bs1, bs2)) | ||
| S_mo = np.linalg.multi_dot([C1.T, S_ao, C2]) | ||
|
|
||
| if o1 != o2: |
There was a problem hiding this comment.
What if o1 and o2 are ever so slightly off? I understand that in practice, we won't encounter that, but please either don't check this case at all, or have a more robuster check to see if those two matrices are similar.
There was a problem hiding this comment.
The function spaces returns the orbital basis set (OBS) and the complementary auxiliary basis set (CABS). By definition, the CABS is the orthogonal complement to the OBS so the overlap of the MOs between the two should be rigorously zero.
There was a problem hiding this comment.
We're talking past each other, so let's back up. Is the condition o1 != o2 ever going to be false? The code tells me both true and false are possible. What you are telling me tells me that o1 != o2 should always be true.
There was a problem hiding this comment.
@pytest.mark.parametrize("o1,o2", [(0, 0), (0, 1), (1, 0), (1, 1)])
The test looks at the orthonormality of the OBS with itself (o1 = 0 and o2 =0), the OBS with the CABS (o1 = 0 and o2 = 1, or vice versa), and the CABS with itself (o1 = 1 and o2 =1). So o1 != o2 is sometimes false.
There was a problem hiding this comment.
Okay, that makes much more sense. I'm used to different parameter values being entirely different computations, not different checks on a single computation. Can you please add a docstring explaining what this test does, so that doesn't bite anybody else? Example: "Test that the CABS and OBS spaces are orthonormal." Renaming "o1" and "o2" to "idx1" and "idx2" would also make a lot of difference.
|
I'll happily approve after these final comments. Unless anybody else objects, I can merge this in last. Thanks again for your patience! |
Just want to make sure this one didn't get lost. 😅 |
|
The reason for docs build failure is unrelated to this PR. I've asked Lori to take a look. |
|
Almost certainly because Ben took down some old qcel/qcng docs in preparation for the qcfractal release. I'll hunt down the replacement. Probably this can still join the merge queue b/c only Azure is required to pass. |
Description
Provides users the ability to form a basisset composed of two combined basissets, the two can be combined simply or through use of the complementary auxiliary basis set (CABS) approach of Valeev et al.
Dev notes & details
pyconstruct_combinedin qcdb with minor changes to fit updated pythonbuild_ri_spacein OrbitalSpace to require a prebuilt combinedBasisSetobjectbuild_cabs_spacein OrbitalSpace to use a full SVDtest_orbitalspace.pyto check orthonormality between orbital basis set and CABSQuestions
_pybuild_basisakaBasisSet.buildto work with a list of keys, targets, roles, and other? Or create a new function to more easily access the building of a combined basisset?Checklist
test_orbitalspace.pyStatus