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

Implement fixed reference variant of DirectJK. #3111

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

Conversation

susilehtola
Copy link
Member

@susilehtola susilehtola commented Dec 16, 2023

Description

This is the variant of DirectJK I mentioned at the end of @davpoolechem's talk at PsiCon last Saturday. The idea is simple: instead of updating the reference every iteration, which is subject to creeping numerical noise, use a fixed reference density and JK matrix for all iterations.

I also simplified some of the DirectJK code, and added missing documentation.

User API & Changelog headlines

  • RN 1
  • RN 2

Dev notes & details

  • Feature1
  • Feature2

Questions

  • Should full formation still happen every N iterations? I don't know if this truly is less susceptible to numerical noise accumulation...

Checklist

Status

  • Ready for review
  • Ready for merge

if (do_wK_) wK_ao_[jki]->add(wK_reference_[jki]);
}

// If we don't do incremental formation, we can update the reference
Copy link
Contributor

Choose a reason for hiding this comment

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

The main thing I'd like to point out is right here. With this implementation, the update of the reference is tied to the INCFOCK_FULL_FOCK_EVERY keyword, because the reference will only be updated when an IncFock iteration isn't done, i.e., when the full Fock matrix is constructed. This is, of course, great for numerical stability. But, to get consistent reference updates, it forces you to set INCFOCK_FULL_FOCK_EVERY to a smallish value. Psi4 uses a small default value already (5), but one goal of the IncFock changes is to avoid consistent reconstruction of the Fock matrix (for some comparisons, ORCA has a similar keyword set to 15 by default if I recall, and GAMESS never recomputes the full Fock matrix).

Would it be good to decouple the regeneration of the reference from the formation of the full Fock matrices? With such an implementation, some references could be difference matrices themselves, which could be a numerical stability concern. But perhaps this is okay in practice?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not sure I follow what you say, but I'll have another look at the code with clearer eyes when I have the time.

One thing one could do with the changes is to use the fixed-reference variant in the background even when one uses traditional incremental formation: the "full" rebuild would be done with respect to the fixed reference matrix, which would make the consecutive full rebuilds much faster, as one would only need to compute the full matrix once.

Copy link
Member Author

Choose a reason for hiding this comment

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

Should be fixed now, still need to think about how to do the combination

@davpoolechem
Copy link
Contributor

Thank you for this PR! I greatly appreciate it!

The cleanup is quite nice, and pretty necessary honestly. Aside from a few small doc wording suggestions, my biggest thought is regarding coupling the regeneration of the reference matrices to construction of the full Fock matrix.

doc/sphinxman/source/scf.rst Outdated Show resolved Hide resolved
doc/sphinxman/source/scf.rst Outdated Show resolved Hide resolved
Co-authored-by: Lori A. Burns <lori.burns@gmail.com>
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