CompositeJK Part 1.5: DFJLinK/DFJCOSK IncFock Standardization #2792

davpoolechem · 2022-11-23T15:47:20Z

Description

This PR is a rather important update to the DFJLinK JK subclass, counting both as the continuation of the CompositeJK development started in #2762 and the first PR in the new incremental Fock build standardization seen in #2682.

To describe the problem, as of now, different integral-direct JK algorithms use different implementations of the incremental Fock build formalism. DirectJK and DFJLinK use the same incremental Fock build formalism, while DFJCOSK uses its own methodology. This introduces two problems:

The next step of CompositeJK development is to combine DFJLinK and DFJCOSK into the pilot CompositeJK subclass. To ease this process, DFJLinK and DFJCOSK need to use the same incremental Fock process.
The incremental Fock formalism currently used by DirectJK and DFJLinK has a couple of extra bells and whistles that DFJCOSK does not have - mainly, the ability to recompute the full Fock matrix every couple of iterations and the ability to disable incremental Fock construction entirely past a specific convergence threshold. These bells and whistles can greatly improve the convergence capabilities of the calculation, while "normal" IncFock implementations without these bells and whistles run the risk of greatly increasing the number of SCF iterations needed to converge. This issue is meant to be addressed with Refactor Incremental Fock Builds in Chain-of-Spheres Exchange #2682; however, the introduction of DFJLinK to the JK hierarchy has potentially changed how that PR should be handled.

This PR addresses both of the above issues by changing DFJLinK to use the same incremental Fock formalism as DFJCOSK. Using the DFJCOSK incremental Fock formalism is preferred because DFJCOSK stores fewer matrices in the DFJCOSK class for incremental Fock usage, reducing memory requirements from a practical perspective, and lowering the amount of state contained in DFJLinK from a code design perspective. In standardizing the DFJLinK and DFJCOSK incremental Fock processes, the next CompositeJK PR will be smoother, and CompositeJK development can continue parallel to the developments discussed in #2682. Additionally, this PR serves as a first step the to decomposition of #2682 as discussed in that PR's comments, allowing for the full standardization of IncFock among integral-direct JK subclasses.

For reviewers, since this PR is the bottleneck for two different routes of JK development (CompositeJK and IncFock standardization), it should be considered the highest-priority JK development PR to merge into Psi4 at the moment.

User API & Changelog headlines

N/A

Dev notes & details

Switches DFJLinK to using the incremental Fock build implementation used in DFJCOSK. This change standardizes the incremental Fock implementation between DFJLinK and DFJCOSK and improves the memory usage of DFJLinK in the process.
Changes LinK machinery to support new incremental Fock formalism.

Questions

Since this PR specifically focuses on changes to DFJLinK, there are still a couple of small differences in IncFock between DFJLinK and DFJCOSK, mainly, refactorings and the aforementioned bells and whistles that DFJLinK has that DFJCOSK doesn't. Would it be better to make adjustments to DFJCOSK in this PR as well, to further standardize the two; or is it preferrable to punt that down to the next CompositeJK PR, when DFJLinK and DFJCOSK are combined into the pilot CompositeJK implementation? If we decide to make the changes to DFJCOSK this PR, it would essentially consist of refactoring the DFJCOSK IncFock to match that of DFJLinK, and adding the bells and whistles that DFJLinK currently has.
Should the relevant test outputs be updated?

Checklist

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

Status

Ready for review
Ready for merge

davpoolechem · 2022-11-23T16:12:57Z

Something that has been brought to my attention that I should clarify and haven't - the bells and whistles that DFJLinK used to have with its IncFock formalism are still there; this PR does not remove those. Rather, this PR updates DFJLinK's IncFock to be as close to DFJCOSK's IncFock as possible while keeping the bells and whistles.

This is also where my first question comes from - would it be good to fully standardize the two by changing DFJCOSK's IncFock in this PR, as well?

loriab

This lgtm after some whitespace tidying for clarity. Would it be useful to add a check to existing tests of each of the JK classes that can use incfock along the lines of compare(1, variable("SCF ITERATIONS") < 15.0, "incfock efficient")?

psi4/src/psi4/libfock/DFJLinK.cc

davpoolechem · 2022-11-28T15:54:33Z

This lgtm after some whitespace tidying for clarity. Would it be useful to add a check to existing tests of each of the JK classes that can use incfock along the lines of compare(1, variable("SCF ITERATIONS") < 15.0, "incfock efficient")?

Thanks; whitespace issues cleaned up!

Regarding the test you suggest, I do think that is a good idea. In principle, if we wanted to measure IncFock efficiency in a test, it might be better measured using a difference in SCF iteration counts between IncFock and corresponding non-IncFock runs, rather than using a raw SCF iteration count for the IncFock run. But none of the tests, as far as I am aware, have both IncFock and non-IncFock runs for the same SCF method executed simultaneously. Your suggested method works with the current tests, and it should still work quite well for testing IncFock efficiency since it will catch any cases where IncFock causes serious convergence issues. I will implement it!

davpoolechem · 2022-11-28T16:00:07Z

Also, based on your comment, I just took a look at the SCF iteration counts for linK-1, and it seems IncFock now leads to a higher SCF iteration count then it should. I didn't think I touched the DFJLinK bells and whistles, but maybe I accidentally did somewhere along the way. Let me resolve this before merging in this PR.

davpoolechem · 2022-11-28T17:09:30Z

All right, bug fixed, and IncFock tests added! The new IncFock efficiency tests compare against an SCF ITERATIONS value of x+3, where x is the number of SCF iterations observed in the corresponding non-IncFock run (which also matches the iteration count seen in each output.ref file). I can reduce the 3 component down to something else if desired.

loriab · 2022-11-28T18:05:39Z

psi4/src/psi4/libfock/DFJLinK.cc


 void DFJLinK::incfock_setup() {
-
-    // The prev_D_ao_ condition is used to handle stability analysis case


Did the stability analysis case get checked? I don't know right off if it was attached to any tests.

I just finished checking this, using the linK-1 test as my test case. Using STABILITY_ANALYSIS=CHECK with RHF and ROHF references, the stability analysis runs to completion. However, for STABILITY_ANALYSIS=CHECK with a UHF reference, the calculation segfaults after the SCF iterations converge. As it were, this issue seems to exist with DFJLinK in the base version of Psi4 at the moment, as well.

Thanks for checking. I think UHF is what @JonathonMisiewicz has moved py-side recently. We can trap the combination if there's not a clean fix.

My guess is that some save_jk flags are needed by the Py-side Davidson tech, and that wasn't properly documented. Based on what David says, that seems unrelated to this PR.

Co-authored-by: Lori A. Burns <[email protected]>

JonathonMisiewicz

Leaving merge rights to @loriab.

loriab added the scf Involves general SCF: convergence algorithms, RHF/UHF/ROHF/CUHF... label Nov 23, 2022

loriab added this to the Psi4 1.7 milestone Nov 23, 2022

davpoolechem force-pushed the dpoole34/dfjlink-incfock branch from eefaa36 to a004ba6 Compare November 28, 2022 14:32

loriab reviewed Nov 28, 2022

View reviewed changes

psi4/src/psi4/libfock/DFJLinK.cc Outdated Show resolved Hide resolved

psi4/src/psi4/libfock/DFJLinK.cc Outdated Show resolved Hide resolved

psi4/src/psi4/libfock/DFJLinK.cc Outdated Show resolved Hide resolved

psi4/src/psi4/libfock/DFJLinK.cc Outdated Show resolved Hide resolved

loriab reviewed Nov 28, 2022

View reviewed changes

hokru approved these changes Nov 29, 2022

View reviewed changes

davpoolechem force-pushed the dpoole34/dfjlink-incfock branch from 3393728 to c75c415 Compare November 29, 2022 14:02

loriab requested a review from JonathonMisiewicz November 29, 2022 15:34

David Poole and others added 13 commits November 29, 2022 11:44

Start on standardizing DFJLinK/DFJCOSK incfock

48a0eed

New DFJLinK IncFock seems to pass linK-1 test now

cfbcde5

Remove segfault; linK-x and scf5 tests work

1e22e12

Clean up code some

89e6f2c

Get rid of extraneous member variables in DFJLinK

6fe8a1f

Whitespace cleanup

7824185

More whitespace cleanup

9bc19f8

Update psi4/src/psi4/libfock/DFJLinK.cc

585138b

Co-authored-by: Lori A. Burns <[email protected]>

Update psi4/src/psi4/libfock/DFJLinK.cc

e776b71

Co-authored-by: Lori A. Burns <[email protected]>

Update psi4/src/psi4/libfock/DFJLinK.cc

ee608ba

Co-authored-by: Lori A. Burns <[email protected]>

Update psi4/src/psi4/libfock/DFJLinK.cc

c2fef9d

Co-authored-by: Lori A. Burns <[email protected]>

Fix bug causing extra SCF iterations in DFJLinK IncFock

c8e4469

Add incfock efficiency tests to linK-x test runs

7a4ff91

davpoolechem force-pushed the dpoole34/dfjlink-incfock branch from c75c415 to 7a4ff91 Compare November 29, 2022 16:45

davpoolechem mentioned this pull request Nov 29, 2022

IncFock Standardization Part 2: DirectJK #2808

Merged

7 tasks

JonathonMisiewicz approved these changes Nov 30, 2022

View reviewed changes

loriab approved these changes Nov 30, 2022

View reviewed changes

loriab merged commit 2278417 into psi4:master Nov 30, 2022

davpoolechem mentioned this pull request Dec 1, 2022

IncFock Standardization Part 3: DFJCOSK #2816

Merged

12 tasks

davpoolechem mentioned this pull request Apr 11, 2023

Refactor Incremental Fock Builds in Chain-of-Spheres Exchange #2682

Closed

9 tasks


		void DFJLinK::incfock_setup() {

		// The prev_D_ao_ condition is used to handle stability analysis case

CompositeJK Part 1.5: DFJLinK/DFJCOSK IncFock Standardization #2792

CompositeJK Part 1.5: DFJLinK/DFJCOSK IncFock Standardization #2792

Uh oh!

Conversation

davpoolechem commented Nov 23, 2022

Description

User API & Changelog headlines

Dev notes & details

Questions

Checklist

Status

Uh oh!

davpoolechem commented Nov 23, 2022

Uh oh!

loriab left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

davpoolechem commented Nov 28, 2022

Uh oh!

davpoolechem commented Nov 28, 2022

Uh oh!

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

Uh oh!

Uh oh!

loriab Nov 28, 2022

Choose a reason for hiding this comment

Uh oh!

davpoolechem Nov 29, 2022

Choose a reason for hiding this comment

Uh oh!

loriab Nov 29, 2022

Choose a reason for hiding this comment

Uh oh!

JonathonMisiewicz Nov 30, 2022

Choose a reason for hiding this comment

Uh oh!

JonathonMisiewicz left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

davpoolechem commented Nov 28, 2022 •

edited

Loading