-
Notifications
You must be signed in to change notification settings - Fork 440
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
Density Screening Refactor Part 1: test_erisieve.py Rework #2547
Conversation
Details about how the integrals were computed should be the province of the JK object, not the HF wavefunction, so I disagree with creating this new variable as described. Can we instead have |
That should definitely be doable! Give me a bit, and that change can be made. |
3a87bee
to
0058940
Compare
Done and done! computed_shells_per_iter_ is now in the JK object. |
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 have two minor code cleanup requests, but LGTM otherwise.
Short PRs are appreciated!
b0f0263
to
be00cd0
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.
LGTM, no comments from me
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.
Looks good
psi4/src/export_wavefunction.cc
Outdated
@@ -394,7 +394,9 @@ void export_wavefunction(py::module& m) { | |||
"Are we to do excited-state MOM?") | |||
.def_property("MOM_performed_", &scf::HF::MOM_performed, &scf::HF::set_MOM_performed, | |||
"MOM performed current iteration?") | |||
.def_property("attempt_number_", &scf::HF::attempt_number, &scf::HF::set_attempt_number, | |||
.def_property("computed_shells_per_iter_", &scf::HF::computed_shells_per_iter, &scf::HF::set_computed_shells_per_iter, | |||
"Array containing the number of shells computed (not screrened out) during each SCF iteration.") |
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.
"Array containing the number of shells computed (not screrened out) during each SCF iteration.") | |
"Array containing the number of shell quartets computed (not screened out) during each SCF iteration.") |
tests/pytests/test_erisieve.py
Outdated
schwarz_computed_shells_expected = [20290, 20290, 20290, 20290, 20290, 20290, 20290, 20290, 20290] | ||
density_computed_shells_expected = [13171, 19618, 19665, 19657, 19661, 19661, 19663, 19663, 19663] | ||
|
||
for iter_ in range(0,9): |
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.
It would be good to explicitly check that the lists of expected and computed shell quartets are the same length (the number of SCF iterations, which has been hardcoded as 9 here).
Better yet, I think there's a compare_arrays
function that you could call instead of repeated calls to compare_integers
.
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.
good point. compare_arrays
is legacy name -- you can use compare_values
directly. It handles float-like, while compare
handles int-like.
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.
This is indeed a very good point. I will make these changes!
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 right, this change should be made now!
tests/pytests/test_erisieve.py
Outdated
assert compare_integers(screen_count_uhf, 19440, 'UHF Density Screened Ints Count, Cutoff 1.0e-12') | ||
computed_shells_expected = [13171, 19618, 19665, 19657, 19661, 19661, 19663, 19663, 19663] | ||
|
||
for iter_ in range(0,9): |
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.
Same as above comment
0b27386
to
d7bfd8c
Compare
Co-authored-by: Zach Glick <glickzachary@gmail.com>
So I now realize something - we may want to apply some of the benchmarking changes made in this PR to DFJCOSK, as well. It will increase the size of the PR, but the benchmarking changes in this PR currently only extend to DirectJK at the moment. Since DFJCOSK has two methods that it separately benchmarks, it will require a bit of retooling regarding some of the internals of the benchmarking framework. It should not have a significant impact on test_erisieve, however. Thoughts? |
Unless the DFJCOSK changes would undo much of this PR, I think a follow-up PR would be best. |
DFJCOSK won't explicitly undo most of this PR, nicely enough, though it will require some changes to how the computed_shells member functions/variables are handled. Regardless, it won't lead to significant changes in test_erisieve, so a separate PR should work fine. And ultimately, the big point of this PR is to allow testing of density screening in test_erisieve without needing to directly construct and use separate TwoBodyAOInt objects, since the plan is to remove density screening from TwoBodyAOInt entirely. Thank you for your feedback! |
Description
This PR is the first in a series of planned PRs designed to remove density screening from the TwoBodyAOInt object and into the JK object. Having density screening available in TwoBodyAOInt runs the risk of applying density screening to algorithms where density screening doesn't make sense. Thus, it would be a good idea to move the logic of density screening to where it is more correctly applied, i.e., the JK object.
This PR solves two issues simultaneously:
The primary purpose of this PR is to change the test_erisieve.py tests to work with the planned future density screening refactor. One issue that moving density screening from TwoBodyAOInt to JK currently brings up, is that it causes the tests on density screening within the pytest test_erisieve.py to fail. These failures occur because test_erisieve.py performs its screening tests directly using an ERI object generated by IntegralFactory. With density screening being removed from the TwoBodyAOInt object, this method of density screening testing can no longer be done. The current PR is designed to address this issue for when the density screening refactor happens. The aforementioned issue is addressed by implementing a new variable to the HF wavefunction, computed_shells_per_iter_, which keeps track of the number of shell quartets computed per SCF iteration. The computed_shells_per_iter_ variable is accessible to the user via Python, and thus can be used to conduct screening tests. In this way, density screening tests can be performed without the need for an ERI object.
As a bonus from the changes introduced by this PR, the DirectJK algorithm no longer has a need to print computed shell quartet counts to bench.dat. Bench.dat is used exclusively by the DirectJK object to dump the number of shell quartets computed per SCF iteration somewhere. That data is now accessible to the user in a cleaner fashion - it can be accessed through Python, in a manipulatable format.
Notes
Todos
Questions
Checklist
Status