Skip to content
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

Add Comprehensive JK Build+Screening Testing #2978

Merged

Conversation

davpoolechem
Copy link
Contributor

@davpoolechem davpoolechem commented Jun 5, 2023

Description

This PR is designed to enable testing of a wide variety of combinations of SCF_TYPE, SCF_SUBTYPE, and SCREENING keywords available in Psi4. Of the currently-available JK tests, scf5 covers a wide variety of build methods, but only at a single screening type per method (density or csam, depending on the method). Meanwhile, test_erisieve.py, after the updates introduced in #2973, tests a wide variety of screening types, but with limited testing in conjunction with different JK builds (the only tests that don't use the Python interface of TwoBodyAOInt directly, use SCF_TYPE=DIRECT or DF as the JK method for screening comparisons). This leaves a lot of untested JK build+screening combos, which may be potentially broken and uncaught by the CI as a result. As a matter of fact, such cases actually do exist in the code currently (e.g., CompositeJK methods + no screening).

This PR adds a new pytest module, test_comprehensive_jk_screening.py. It is effectively an expanded version of the scf5 test module, testing one of the scf5 systems (singlet oxygen) with the same basis set, but also including different screening methods and algorithmic subtypes available in Psi4. Screening is assumed to have an insignificant impact on energy within the tolerance used, so all screening types for a given method use the same reference energy. Some combinations of method and screening type throw an exception by design; this is accounted for in the test by testing that such combinations do indeed throw an exception as expected. Other combinations of method and algorithm are broken at the moment and error out; these are simply skipped for now. They are all logged in the same spot, and can and will be addressed in future PRs.

User API & Changelog headlines

  • N/A

Dev notes & details

  • Adds a new pytest module to Psi4, test_comprehensive_jk_screening.py , to test different combinations of JK build algorithms and ERI screening methods.

Questions

  • Would the test in test_comprehensive_jk_screening.py be better placed in test_erisieve.py? I placed the test in the former because I considered it large enough to warrant not having the quick pytest mark, but I'm ambivalent about where the test goes between those two test modules.

Checklist

Status

  • Ready for review
  • Ready for merge

@davpoolechem davpoolechem force-pushed the dpoole34/comprehensive-screentest branch from a1be18f to 3df8715 Compare June 6, 2023 12:08
Copy link
Member

@loriab loriab left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm, thanks!

"DF" : -149.58715054487624, #TEST
"Composite": {
"DFDIRJ+COSX" : -149.58722317236171, #TEST
"DFDIRJ+LINK" : -149.58726772171027 #TEST
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fwiw, the #TEST don't do anything in tests/pytests/

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, good to know. I will probably remove it, then.

@loriab loriab added this to the Psi4 1.9 milestone Jun 7, 2023
@loriab loriab added the testing label Jun 7, 2023
@davpoolechem davpoolechem force-pushed the dpoole34/comprehensive-screentest branch from b8b86d7 to cb46252 Compare June 12, 2023 12:10
@davpoolechem davpoolechem force-pushed the dpoole34/comprehensive-screentest branch 2 times, most recently from f29b27e to f68250f Compare June 26, 2023 13:18
@davpoolechem davpoolechem force-pushed the dpoole34/comprehensive-screentest branch 2 times, most recently from eab9630 to 9c91e56 Compare July 3, 2023 16:58
@davpoolechem davpoolechem force-pushed the dpoole34/comprehensive-screentest branch 2 times, most recently from 3cf1210 to cb344c1 Compare July 11, 2023 15:18
@davpoolechem davpoolechem force-pushed the dpoole34/comprehensive-screentest branch from d7e7e7e to d0771ea Compare July 20, 2023 13:25
@davpoolechem davpoolechem force-pushed the dpoole34/comprehensive-screentest branch 3 times, most recently from b79c987 to 5dbfbb3 Compare August 14, 2023 13:41
@davpoolechem davpoolechem force-pushed the dpoole34/comprehensive-screentest branch 2 times, most recently from 3a47b78 to 963dc36 Compare August 28, 2023 13:20
@davpoolechem davpoolechem force-pushed the dpoole34/comprehensive-screentest branch from 963dc36 to 98d4937 Compare September 5, 2023 13:19
@davpoolechem davpoolechem force-pushed the dpoole34/comprehensive-screentest branch from 98d4937 to 69457fc Compare September 18, 2023 12:10
@davpoolechem davpoolechem force-pushed the dpoole34/comprehensive-screentest branch from 69457fc to 471fbd2 Compare September 26, 2023 13:27
@davpoolechem davpoolechem force-pushed the dpoole34/comprehensive-screentest branch from 471fbd2 to c92e18b Compare October 2, 2023 13:12
@davpoolechem davpoolechem added this pull request to the merge queue Oct 2, 2023
Merged via the queue into psi4:master with commit 5659cd1 Oct 2, 2023
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants