IncFock Standardization Part 2: DirectJK #2808

davpoolechem · 2022-11-29T19:37:06Z

Description

This PR is a continuation of the IncFock standardization effort initially started in #2682, and restarted in #2792. It is the second PR listed in the PR submission scheme discussed in the comments in #2682.

For all intents and purposes, the PR has basically the same changes as #2792, except applied to DirectJK instead of DFJLinK. The same benefits to DFJLinK seen in #2792 are applied to DirectJK here, and many of the same points of the benefits of that PR apply here, as well.

A couple of notes to be had for reviewers, some of which arise from discussions in #2792:

Unlike CompositeJK Part 1.5: DFJLinK/DFJCOSK IncFock Standardization #2792, this PR is independent of the CompositeJK effort, since this PR is localized within DirectJK.
This PR makes slightly more expansive changes to the DirectJK infrastructure to support the new IncFock formalism, but it is still not a massive number of changes; and most of these changes are similar to those observed in DFJLinK in CompositeJK Part 1.5: DFJLinK/DFJCOSK IncFock Standardization #2792.
As with CompositeJK Part 1.5: DFJLinK/DFJCOSK IncFock Standardization #2792, this PR does not remove the bells and whistles related to IncFock seen in DirectJK (e.g., recomputing the full Fock matrix every so often, disabling IncFock past a given convergence threshold). This PR incorporates the DFJCOSK IncFock formalism as much as possible into DirectJK, while maintaining the IncFock bells and whistles seen in DirectJK.
It should be noted that there are no directJK-x tests akin to the linK-x tests, and thus, no IncFock efficiency tests were added in this PR, as was done for DFJLinK in CompositeJK Part 1.5: DFJLinK/DFJCOSK IncFock Standardization #2792. Technically, IncFock efficiency tests could be added to scf5, but I feel that goes against the spirit of the scf5 test.
I looked at stability tests, as well. STABILITY_ANALYSIS=CHECK works for all of RHF, ROHF, and UHF references.

User API & Changelog headlines

N/A

Dev notes & details

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

Questions

Do we want to add extra tests for DirectJK to allow for Incremental Fock efficiency tests?

Checklist

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

Status

Ready for review
Ready for merge

loriab

Thanks for looking at stability. Probably later we can add a test for each of the stabilities that parameterizes all the scf methods and incfock combinations.

loriab · 2022-11-30T07:14:09Z

psi4/src/psi4/libfock/DirectJK.cc

 #ifdef USING_BrianQC
    if (brianEnable) {
+        // zero out J, K, and wK matrices
+        zero();


just checking that editing brianqc code intentional.

Indeed it is!

The original DirectJK code has a call to zero() at the very beginning of compute_JK(). Unfortunately, this zero() call breaks the IncFock code when using the new IncFock formalism, so essentially, the call to zero() was moved down further in DirectJK. However, the call to zero() also occurs before the BrianQC code. So, I assumed that just flat-out getting rid of the call to zero() at the beginning would break the BrianQC code. The addition of zero() to BrianQC, then, is essentially making sure that zero() is still called at the beginning when BrianQC is used.

Thinking about it further, though, it's possible this same effect can be had by just replacing the original call to zero() with something like

if (!incfock_) zero();

Then, the zero() call is only done for non-IncFock runs without changing the BrianQC code at all. Might be safer to do it that way, since I am not sure if we have immediate access to BrianQC to test changes to BrianQC code.

We don't have immediate BrianQC access, so I think the consolidated zero() you propose would be good.

I'll admit, my only concern is some edge case where someone uses BrianQC while requesting IncFock, then the initial zeroing doesn't happen because IncFock is requested, and then BrianQC possibly breaks.

davpoolechem · 2022-11-30T14:52:06Z

Thanks for looking at stability. Probably later we can add a test for each of the stabilities that parameterizes all the scf methods and incfock combinations.

You're welcome! This would probably be a good idea, especially since, from my testing, some issues pop up with stability tests UHF references depending on the SCF_TYPE method used currently.

JonathonMisiewicz · 2022-11-30T15:31:57Z

Ping me for review after Pt 1.5 gets merged in. I'm worried about possible merge conflicts.

JonathonMisiewicz

This all looks good to me. Let us know when this is ready to merge.

davpoolechem · 2022-11-30T16:04:40Z

This all looks good to me. Let us know when this is ready to merge.

Will do! I think I want to run a quick final test set with the DFJLinK PR merged in, to make sure nothing broke. Then, it should be good to go!

davpoolechem · 2022-11-30T18:38:35Z

So the CI error here is... interesting.

FAILED: src/psi4/libdpd/CMakeFiles/dpd.dir/buf4_mat_irrep_wrt.cc.obj C:\PROGRA~1\LLVM\bin\clang-cl.exe /nologo -TP -DUSING_LAPACK_MKL -D_CRT_NONSTDC_NO_DEPRECATE -D_CRT_NONSTDC_NO_WARNINGS -D_CRT_SECURE_NO_WARNINGS -D_USE_MATH_DEFINES -ID:\a\1\s\psi4\include -ID:\a\1\s\psi4\src -imsvc C:\tools\miniconda3\include -imsvc C:\tools\miniconda3\Library\include -imsvc C:\tools\miniconda3\Library\include\eigen3 /arch:AVX /O2 /Ob2 /DNDEBUG -MD /EHsc -Xclang -fopenmp -std:c++17 /showIncludes /Fosrc\psi4\libdpd\CMakeFiles\dpd.dir\buf4_mat_irrep_wrt.cc.obj /Fdsrc\psi4\libdpd\CMakeFiles\dpd.dir\dpd.pdb -c -- D:\a\1\s\psi4\src\psi4\libdpd\buf4_mat_irrep_wrt.cc In file included from D:\a\1\s\psi4\src\psi4\libdpd\buf4_mat_irrep_wrt.cc:33: In file included from C:\Program Files\Microsoft Visual Studio\2022\Enterprise\VC\Tools\MSVC\14.34.31933\include\cstdio:9: C:\Program Files\Microsoft Visual Studio\2022\Enterprise\VC\Tools\MSVC\14.34.31933\include\yvals_core.h(784,1): error: static_assert failed "Error in C++ Standard Library usage." _EMIT_STL_ERROR(STL1000, "Unexpected compiler version, expected Clang 14.0.0 or newer."); ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ C:\Program Files\Microsoft Visual Studio\2022\Enterprise\VC\Tools\MSVC\14.34.31933\include\yvals_core.h(456,5): note: expanded from macro '_EMIT_STL_ERROR' static_assert(false, "Error in C++ Standard Library usage.") ^ ~~~~~ 1 error generated.

Two overall errors on the Windows Psi4 build in this vein.

loriab · 2022-11-30T22:38:52Z

I've gotten that once today, too. Then I've gotten distracted by other matters. Even if a restart fixes it, another CI failure will crop up later on, so please rebase, and we'll see if this is a consistent issue.

loriab · 2022-12-01T05:14:36Z

I got the static_assert error again now. The file it's triggering on hasn't had recent changes, so I don't know why it's popping up now, unless it's an intermittant/brownout strategy to push upgrades. Anyways, switching the llvm version https://github.com/psi4/psi4/pull/2791/files#diff-9f5f8e4d87a7d1fbb0e8f30d1c680c5f42f6af10c04aa81d342c7dfa5af9a99fR10 at least gets the build started.

davpoolechem · 2022-12-01T14:29:39Z

I got the static_assert error again now. The file it's triggering on hasn't had recent changes, so I don't know why it's popping up now, unless it's an intermittant/brownout strategy to push upgrades. Anyways, switching the llvm version https://github.com/psi4/psi4/pull/2791/files#diff-9f5f8e4d87a7d1fbb0e8f30d1c680c5f42f6af10c04aa81d342c7dfa5af9a99fR10 at least gets the build started.

Hmmmm, I see. Thank you for looking into this! I just rebased and pushed again, so we will see if that helps.

loriab · 2022-12-01T15:34:50Z

The LLVM bump was a trial in a branch, not master, so you'll have to edit, not just rebase. Sorry I was unclear.

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

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

loriab approved these changes Nov 30, 2022

View reviewed changes

loriab requested a review from JonathonMisiewicz November 30, 2022 15:59

JonathonMisiewicz approved these changes Nov 30, 2022

View reviewed changes

davpoolechem force-pushed the dpoole34/directjk-incfock branch from 7943e4f to fde9a15 Compare November 30, 2022 17:13

David Poole added 8 commits December 1, 2022 09:21

Start DirectJK IncFock standardization

e6e7e94

Fix bug causing extra DirectJK IncFock iterations

a0d43d2

Remove incfock testing from scf5 sample

6fb85ec

Whitespace changes

9544ccf

Fix some issues leading to wrong answers

2fcef69

Fix some spacing issues

1302939

Fix a couple of failed tests, more to come

ed5426d

Fix bug causing rest of test fails

772eea5

davpoolechem force-pushed the dpoole34/directjk-incfock branch from fde9a15 to 772eea5 Compare December 1, 2022 14:25

Update LLVM version from 13.0 to 14.0; hopefully this fixes CI issues

d329fa8

davpoolechem mentioned this pull request Dec 1, 2022

IncFock Standardization Part 3: DFJCOSK #2816

Merged

12 tasks

loriab merged commit 727a3b9 into psi4:master Dec 1, 2022

davpoolechem mentioned this pull request Apr 11, 2023

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

Closed

9 tasks

IncFock Standardization Part 2: DirectJK #2808

IncFock Standardization Part 2: DirectJK #2808

Uh oh!

Conversation

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

Uh oh!

Description

User API & Changelog headlines

Dev notes & details

Questions

Checklist

Status

Uh oh!

loriab left a comment

Choose a reason for hiding this comment

Uh oh!

loriab Nov 30, 2022

Choose a reason for hiding this comment

Uh oh!

davpoolechem Nov 30, 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!

loriab Nov 30, 2022

Choose a reason for hiding this comment

Uh oh!

davpoolechem Nov 30, 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!

davpoolechem commented Nov 30, 2022

Uh oh!

JonathonMisiewicz commented Nov 30, 2022

Uh oh!

JonathonMisiewicz left a comment

Choose a reason for hiding this comment

Uh oh!

davpoolechem commented Nov 30, 2022

Uh oh!

davpoolechem commented Nov 30, 2022

Uh oh!

loriab commented Nov 30, 2022

Uh oh!

loriab commented Dec 1, 2022

Uh oh!

davpoolechem commented Dec 1, 2022

Uh oh!

loriab commented Dec 1, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

davpoolechem commented Nov 29, 2022 •

edited

Loading

davpoolechem Nov 30, 2022 •

edited

Loading

davpoolechem Nov 30, 2022 •

edited

Loading