Density Screening Refactor Part 2: Implementation of shell_significant() #2695

davpoolechem · 2022-08-19T18:13:56Z

Description

This PR is the second in a series of planned PRs designed to remove density screening from the TwoBodyAOInt object and into the JK object, with PR #2547 being the first such PR. 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.

The primary purposes of this PR are twofold:

First, this PR introduce the shell_significant() framework to the JK class. The shell_significant() framework starts with a shell_significant() virtual function that exists in the base JK class. The shell_significant() function can then be redefined specifically for different JK derived classes as needed. The existence of shell_significant() provides a unified framework for performing screening for any JK method, and it also provides the method by which density screening can be added directly to the relevant JK classes.
Second, this PR uses the shell_significant() framework to reimplement screening for certain JK methods. Most significantly, as implied in the first point, the biggest change in this regard was the removal of shell_significant_density() from TwoBodyAOInt into the domain and its reimplementation into the DirectJK shell_significant() definition. With this change, density screening is now the domain of individual JK classes rather than the TwoBodyAOInt class, which was one of the primary goals of this overall refactor PR set in the first place. Additionally, another method that was subject to slight changes from the shell_significant() framework was the Yoshimine PKJK algorithm, in which its test for shell significance was implemented using the shell_significant() framework.

Notes

I wanted to outline what the next steps were for this chain of PRs, since this one accomplishes one of the main goals of the refactor in the first place:

Separate out density screening from the SCREENING keyword and into its own keyword (likely something like DENSITY_SCREENING)
Removal of any density matrix function/variable from TwoBodyAOInt. Without density screening in TwoBodyAOInt, these density matrix references in TwoBodyAOInt are unnecessary and more properly placed into JK, as well.

Todos

Implementation of shell_significant() framework in JK class to represent shell significance testing.
Removal of shell_significant_density() from TwoBodyAOInt.
Reimplementation of density screening in DirectJK via the shell_significant framework.
Reimplementation of density screening in other JK methods via the shell_significant framework.
Alteration of other shell quartet screening implementations in other JK algorithms via the JK framework.

Questions

The shell_significant framework has not yet been added to DFJCOSK. Should that be done this PR, or added in a later PR?

Checklist

Tests added for any new features
All or relevant fraction of full tests run

Status

Ready for review
Ready for merge

andyj10224

Looks good, I left some comments. Namely, my biggest concern is about removing the screening density keyword and moving update_density away from TwoBodyAOInt. I think it would make more sense to get it all done in one PR, but that is my personal opinion.

andyj10224 · 2022-08-19T20:39:59Z

psi4/src/psi4/libmints/twobody.cc

@@ -77,7 +77,7 @@ TwoBodyAOInt::TwoBodyAOInt(const IntegralFactory *intsfactory, int deriv)
    else if (screentype == "CSAM")
        screening_type_ = ScreeningType::CSAM;
    else if (screentype == "DENSITY")


Would it make sense to diable setting screening density altogether?

The reason this is still in here is to account for cases where density screening is used in the HF step and another form of screening is used through TwoBodyAOInt for a later step in the calculation. Looking through the code, it seems that an example of this is in the DirectJKGrad class, which uses TwoBodyAOInt screening functions in its tests for shell block significance.

This will certainly be removed once density screening is separated out as a keyword, though.

psi4/src/psi4/libmints/twobody.h

andyj10224 · 2022-08-19T20:42:19Z

psi4/src/psi4/libmints/twobody.cc

-    // The density screened ERI bound (Eq. 6)
-    return (mn_mn * rs_rs * max_density * max_density >= screening_threshold_squared_);
-}
+double TwoBodyAOInt::shell_pair_max_density(int i, int M, int N) const  {


Should we move shell_pair_max_density to JK in this PR or the next? I don't think this belongs in TwoBodyAOInt

My plan was to move this whenever update_density gets moved, which I'm planning on being the next PR.

davpoolechem · 2022-08-19T21:12:27Z

Looks good, I left some comments. Namely, my biggest concern is about removing the screening density keyword and moving update_density away from TwoBodyAOInt. I think it would make more sense to get it all done in one PR, but that is my personal opinion.

Thank you very much for the comments! I do appreciate it. As for your points, my thought was to implement both of these changes in future PRs, maybe as a single future PR, maybe as their own separate ones. I think there's a good argument to be made for making a separate option for density screening in this PR, and I'd be quite willing to do that. But, I do believe that removing update_density should be its own PR, as that PR would consist of removing ALL references to the density matrix from TwoBodyAOInt, and I think it would be easier to review and clearer intent if update_density was separated out in its own PR.

loriab · 2022-08-19T21:34:59Z

psi4/src/psi4/libfock/PKmanagers.cc

@@ -1111,7 +1118,7 @@ void PKMgrYoshimine::compute_integrals(bool wK) {
            for (size_t j = 0; j <= i; ++j) {
                int RR = sh_pairs[j].first;
                int SS = sh_pairs[j].second;
-                if (tb[thread]->shell_significant(PP, QQ, RR, SS)) {
+                if (shell_significant(PP, QQ, RR, SS)) {


so why does this no longer get applied per-thread? (read as naive, not insightful, question)

The shell_significant function in PKMgrYoshimine is defined to do its shell significance test using the sieve_ variable in the base PKManager class instead of through the TwoBodyAOInt object. Theoretically, shell_significant in PKMgrYoshimine could be redefined to use tb[thread]'s shell_significant definition, though it would be somewhat clumsy since it would require passing { tb[thread] } (or something like that, basically just a vector with only tb[thread] in it) as the optional argument for int.

Talking about it here though makes me wonder whether using sieve_ is the best approach for the shell_significant definition, however. I tested it a while back when I first implemented it, and the screened shell quartet counts matched up between this PR's implementation and the previous one, but I really may want to recheck that for my own sake.

The ERISieve class is deprecated in favor of having the TwoBodyAOInt object do the sieving. This was originally decided as part of the L2 discussion and has been reaffirmed on Slack just a week or two ago. I was in the process of getting rid of ERISieve when this PR came up.

We'll need to agree about the future of "what class is responsible for sieving" before this PR can be merged in.

loriab · 2022-08-19T22:06:03Z

undefined symbol: _ZNK3psi12TwoBodyAOInt22shell_pair_max_densityEiii in case you hadn't clicked through to it yet

davpoolechem · 2022-08-19T22:12:47Z

undefined symbol: _ZNK3psi12TwoBodyAOInt22shell_pair_max_densityEiii in case you hadn't clicked through to it yet

I did indeed notice that! I think I know the problem, so I will apply the fix in just a little bit.

davpoolechem · 2022-08-19T22:37:30Z

All right, CI issue should be fixed now!

JonathonMisiewicz

Blocking this PR until we can resolve the question of "is TwoBodyAOInt responsible for sieving, or is ERISieve"? Happy to talk about this after ACS Chicago.

JonathonMisiewicz · 2022-08-30T13:50:29Z

FTR, David and I have agreed to pause this PR until after #2682 and #2665 come in.

davpoolechem · 2022-12-10T12:25:52Z

Blocking this PR until we can resolve the question of "is TwoBodyAOInt responsible for sieving, or is ERISieve"? Happy to talk about this after ACS Chicago.

To add an update to this, some of us (Jonathan, I, and others) discussed this issue at PsiCon 2022. We came to the conclusion that TwoBodyAOInt would be responsible for sieving, and that ERISieve will be removed from Psi4. As use of ERISieve is seemingly localized to the PKJK algorithm, I will be the one responsible for its removal.

Until then, this PR will likely be further delayed.

…ctJK

…ework

Co-authored-by: Andy Jiang <60559795+andyj10224@users.noreply.github.com>

This reverts commit b6d1def.

andyj10224 · 2024-06-24T19:24:02Z

@davpoolechem Since #3098 has been opened, should we close this PR?

davpoolechem · 2024-08-01T15:26:58Z

@davpoolechem Since #3098 has been opened, should we close this PR?

You are correct, we should! And so I will.

davpoolechem force-pushed the dpoole34/shell-significant branch from 235d939 to 566ba18 Compare August 19, 2022 18:20

andyj10224 reviewed Aug 19, 2022

View reviewed changes

loriab reviewed Aug 19, 2022

View reviewed changes

JonathonMisiewicz requested changes Aug 20, 2022

View reviewed changes

loriab added this to the Psi4 1.8 milestone Nov 28, 2022

davpoolechem mentioned this pull request Apr 28, 2023

Begone, ERISieve! #2933

Closed

10 tasks

loriab modified the milestones: Psi4 1.8, Psi4 1.9 May 4, 2023

davpoolechem force-pushed the dpoole34/shell-significant branch from ad4722e to bdf3073 Compare July 11, 2023 14:27

David Poole and others added 15 commits July 20, 2023 10:40

Start moving density screening out of TwoBodyAOInt; implement in Dire…

52b319a

…ctJK

Move shell_significant_density out of TwoBodyAOInt and into DirectJK

4ebb5f4

Change Yoshimine PKJK algorithm to use new JK::shell_significant fram…

6575c88

…ework

Remove any shell_significant changes from LinK

3f207ed

Fix bug in DirectJK::shell_significant

dee8f0f

Fix compiler error in previous bugfix

10de757

Add a couple of TODOs

38c80dd

Clean up code some

35ac2e6

Rework DirectJK::shell_significant

cfecdec

Change capture value of DirectJK::shell_significant lambda

87c668e

Some more code cleanup

e5aeff1

Update psi4/src/psi4/libmints/twobody.h

c1a65b2

Co-authored-by: Andy Jiang <60559795+andyj10224@users.noreply.github.com>

Revert "Update psi4/src/psi4/libmints/twobody.h"

16b8247

This reverts commit b6d1def.

Some post-rebase changes to compile

2c201ff

Some retabbing

510c2b6

Initial shell_significant impl for DirectDFJ and LinK

98c7008

davpoolechem force-pushed the dpoole34/shell-significant branch from 47e0c21 to 98c7008 Compare July 20, 2023 14:45

loriab modified the milestones: Psi4 1.9, Psi4 1.10 Nov 14, 2023

davpoolechem mentioned this pull request Dec 8, 2023

Reboot - Density Screening Refactor Part 2: Implementation of shell_significant() #3098

Open

9 tasks

davpoolechem closed this Aug 1, 2024

Density Screening Refactor Part 2: Implementation of shell_significant() #2695

Density Screening Refactor Part 2: Implementation of shell_significant() #2695

Uh oh!

Conversation

davpoolechem commented Aug 19, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Notes

Todos

Questions

Checklist

Status

Uh oh!

andyj10224 left a comment

Choose a reason for hiding this comment

Uh oh!

andyj10224 Aug 19, 2022

Choose a reason for hiding this comment

Uh oh!

davpoolechem Aug 19, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

andyj10224 Aug 19, 2022

Choose a reason for hiding this comment

Uh oh!

davpoolechem Aug 19, 2022

Choose a reason for hiding this comment

Uh oh!

davpoolechem commented Aug 19, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

loriab Aug 19, 2022

Choose a reason for hiding this comment

Uh oh!

davpoolechem Aug 19, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JonathonMisiewicz Aug 20, 2022

Choose a reason for hiding this comment

Uh oh!

loriab commented Aug 19, 2022

Uh oh!

davpoolechem commented Aug 19, 2022

Uh oh!

davpoolechem commented Aug 19, 2022

Uh oh!

JonathonMisiewicz left a comment

Choose a reason for hiding this comment

Uh oh!

JonathonMisiewicz commented Aug 30, 2022

Uh oh!

davpoolechem commented Dec 10, 2022

Uh oh!

andyj10224 commented Jun 24, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

davpoolechem commented Aug 1, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

davpoolechem commented Aug 19, 2022 •

edited

Loading

davpoolechem commented Aug 19, 2022 •

edited

Loading

davpoolechem Aug 19, 2022 •

edited

Loading

andyj10224 commented Jun 24, 2024 •

edited

Loading

davpoolechem commented Aug 1, 2024 •

edited

Loading