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

Fixes to SCF_TYPE + SCF_SUBTYPE + SCREENING Combinations #3060

Open
wants to merge 25 commits into
base: master
Choose a base branch
from

Conversation

davpoolechem
Copy link
Contributor

Description

This PR changes the code to fix the majority of combinations of SCF_TYPE/SCF_SUBTYPE/SCREENING keywords which were previously broken.

It was discovered that there were a lot of combinations of SCF_TYPE, SCF_SUBTYPE, and SCREENING keywords (e.g., SCF_TYPE = {CompositeJK} with SCREENING = NONE) that would break upon use, either with an exception or with a hard error such as a segfault. PR #2978 added testing to detect these broken combinations. This PR actually fixes the broken combinations detected through the added test.

The most notable change as a result of fixing these issues is the handling of how certain variables within the TwoBodyAOInt class are initialized. To enable universal support of JK builds with SCREENING = NONE, an option has been added to manually call the TwoBodyAOInt::create_sieve_pair_info() function via the new wrapper function TwoBodyAOInt::initialize_sieve(). Multiple JK builds rely on values of variables initialized via create_sieve_pair_info() , and initialize_sieve() is now used to initialize the required variables if SCREENING = NONE is set. In this way, SCREENING = NONE can exist with the benefits it provides, while still working with all JK builds. Also, changes are made to ensure that no screening actually occurs in JK builds when SCREENING is set to NONE.

In line with the above, some tests have been expanded. test_comprehensive_jk_screening.py has been changed to account for the SCF_TYPE/SCF_SUBTYPE/SCREENING combinations that were fixed. test_erisieve.py has been updated to more thoroughly test SCREENING=NONE.

It is worth noting that, unfortunately, TwoBodyAOInt::initialize_sieve() must be defined per integral engine, and so the fixes of this PR only work with Libint2. The issues this PR tries to fix, still persist if Simint is used instead.

User API & Changelog headlines

  • N/A

Dev notes & details

  • Adds a new function to TwoBodyAOInt, initialize_sieve, which allows for manual initialization of screening variables via create_sieve_pair_info() needed for JK builds.
  • Uses the above function to fix combinations of SCF_TYPE, SCF_SUBTYPE, and SCREENING which were broken.
  • Updates the test_comprehensive_jk_screening.py and test_erisieve.py test to account for the previously-described fixes.

Questions

-[] Should we keep the initialize_sieve name? I named it as such, because it is basically a light wrapper to manually call create_sieve_pair_info. But the name of "sieve" might be objectionable.

Checklist

Status

  • Ready for review
  • Ready for merge

if (rank) eri[rank] = std::shared_ptr<TwoBodyAOInt>(eri.front()->clone());
if (rank) {
eri[rank] = std::shared_ptr<TwoBodyAOInt>(eri.front()->clone());
if (!eri[rank]->initialized()) eri[rank]->initialize_sieve();
Copy link
Member

Choose a reason for hiding this comment

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

Does clone clone the sieve as well? Should one instead first just make sure eri.front() has been initialized()?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It should, but I will double check. If it does, your suggestion would be a better implementation than what I did here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, it does copy the sieve as well. So I have changed all initialization instances where a clone is involved, to what you suggested!

@@ -68,7 +68,7 @@ LinK::LinK(std::shared_ptr<BasisSet> primary, Options& options) : SplitJK(primar
lr_symmetric_ = true;

// set up LinK integral tolerance
if (options["LINK_INTS_TOLERANCE"].has_changed()) {
if (options["LINK_INTS_TOLERANCE"].has_changed() && options.get_str("SCREENING") != "NONE") {
linK_ints_cutoff_ = options.get_double("LINK_INTS_TOLERANCE");
Copy link
Member

Choose a reason for hiding this comment

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

what does the cutoff default to?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It defaults to 1e-12 if any SCREENING other than NONE is selected; and 0.0 if SCREENING = NONE.


/// handle composite methods
} else if (is_composite) {
auto jk = std::make_shared<CompositeJK>(primary, auxiliary, options);

if (options["INTS_TOLERANCE"].has_changed()) jk->set_cutoff(options.get_double("INTS_TOLERANCE"));
Copy link
Member

Choose a reason for hiding this comment

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

why remove this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This has been moved into CompositeJK::common_init() to better account for behavior when SCREENING=NONE.

@loriab loriab added this to the Psi4 1.9 milestone Nov 2, 2023
@loriab loriab modified the milestones: Psi4 1.9, Psi4 1.10 Nov 14, 2023
@davpoolechem davpoolechem force-pushed the dpoole34/compositejk-screenfix branch 2 times, most recently from 9a97a83 to a4ec5d0 Compare December 8, 2023 14:36
@davpoolechem davpoolechem force-pushed the dpoole34/compositejk-screenfix branch from 8bf8248 to 4d375d4 Compare March 18, 2024 13:01
@davpoolechem davpoolechem force-pushed the dpoole34/compositejk-screenfix branch from 4d375d4 to 8dde823 Compare April 15, 2024 12:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants