Skip to content

Adds user option for ScreeningType::None #2973

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

Merged
merged 2 commits into from
May 31, 2023

Conversation

EricaCMitchell
Copy link
Contributor

Description

Add ScreeningType::None as a user option and sets ScreeningType::Schwarz as the default. This should allow use of mixed basis set types to be used in integrals as well.

User API & Changelog headlines

N/A

Dev notes & details

  • Breaks out of setup_sieve() in twobody.cc before exception can be thrown
  • Adds "NONE" to list of options in read_options.cc

Checklist

Status

  • Ready for review
  • Ready for merge

Copy link
Member

@jturney jturney left a comment

Choose a reason for hiding this comment

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

Looks good!

@davpoolechem
Copy link
Contributor

davpoolechem commented May 31, 2023

So I cross-checked this with the CompositeJK methods, and; as I somewhat suspected might happen, the CompositeJK methods do not work with screening none. SCF_TYPE DFDIRJ+LINK segfaults, while SCF_TYPE DFDIRJ+COSX gives the wrong answer, from my testing.

The composite methods partially implement the screening machinery within themselves instead of relying entirely on the TwoBodyAOInt::shell_significant() function for sieving. Some TwoBodyAOInt member functions, such as TwoBodyAOInt::shell_pair_values (used in DF-DirJ), and TwoBodyAOInt::shell_pair_significant (used in LinK) are used to assist with this. However, these functions rely on values that (specifically, shell_pair_values_, screening_threshold_squared_, and max_integral_) are initialized during the call to TwoBodyAOInt::create_sieve_pair_info, which is never called when screening_type is set to none due to the return upon registering ScreeningType::None for the sieve during TwoBodyAOInt::setup_sieve. That's where I suspect these problems are coming from.

Given this wasn't caught by CI, it may be reasonable to add a new test for different JK methods at different screening types, but that wouldn't be for this PR.

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!

I agree with DP that a parametrize on the test_erisieve going through the scf_types could be a good precaution -- we can schedule that as a group programming segment.

@davpoolechem
Copy link
Contributor

Talking to @loriab some, I'll be happy to let this PR be merged in as-is, and fix up the issues I brought up earlier in other PRs.

@loriab loriab added this pull request to the merge queue May 31, 2023
Merged via the queue into psi4:master with commit c6e4e0e May 31, 2023
@loriab loriab added this to the Psi4 1.9 milestone Jun 6, 2023
@EricaCMitchell EricaCMitchell deleted the no_screening branch June 8, 2023 18:58
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.

4 participants