-
Notifications
You must be signed in to change notification settings - Fork 438
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
CompositeJK Part 2: Pilot Implementation #2833
Conversation
To ease the review process, I am going to go through the diff and mark out what should be prioritized by reviewers, and what should not be. |
doc/sphinxman/source/scf.rst
Outdated
@@ -648,7 +648,9 @@ ERI Algorithms | |||
The key difficulty in the SCF procedure is treatment of the four-index ERI | |||
contributions to the Fock Matrix. A number of algorithms are available in | |||
|PSIfour| for these terms. The algorithm is selected by the |globals__scf_type| | |||
keyword, which may be one of the following | |||
keyword. Some such algorithms consist of a single algorithm applied to |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changes in doc/* files are documentation updates regarding CompositeJK - worth looking at.
@@ -39,6 +39,7 @@ | |||
#include "psi4/liboptions/liboptions.h" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The diff interpreted DFJCOSK.cc as being moved to CompositeJK.cc. A lot of this is renaming DFJCOSK functions to CompositeJK functions, but I will point things out where they appear.
|
||
// => Direct Density-Fitted Coulomb Setup <= // |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This block is just moved to later on in the constructor.
} | ||
timer_off("ERI Computers"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This block is just moved to later on in the constructor.
// solve: Q_init_ = S_an @ S_num_init_^{-1} | ||
Q_init_ = S_an->clone(); | ||
C_DGESV(nbf, nbf, S_num_init.pointer()[0], nbf, ipiv.data(), Q_init_->pointer()[0], nbf); | ||
// => Set up separate J algorithm <= // |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here is where the aforementioned moved blocks of code are moved to.
return 0; // Memory is O(N^2), which psi4 counts as effectively 0 | ||
} | ||
|
||
void DFJCOSK::print_header() const { | ||
void CompositeJK::print_header() const { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The header printing scheme was changed a little bit for CompositeJK - this is worth looking at.
|
||
void DFJCOSK::common_init() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lots of moved code here, but the moving of code, and other changes here, are related to the implementation of the CompositeJK front end.
// Save a copy of the density for the next iteration | ||
D_prev_.clear(); | ||
for(auto const &Di : D_ao_) { | ||
D_prev_.push_back(Di->clone()); | ||
} | ||
} | ||
|
||
void DFJCOSK::compute_JK() { | ||
void CompositeJK::compute_JK() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some changes here to accomodate the new front end.
|
||
void DFJCOSK::build_J(std::vector<std::shared_ptr<Matrix>>& D, std::vector<std::shared_ptr<Matrix>>& J) { | ||
void CompositeJK::build_DirectDFJ(std::vector<std::shared_ptr<Matrix>>& D, std::vector<std::shared_ptr<Matrix>>& J) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The changes here really only fully standardize Direct DF-J between LinK and COSX, because DFJCOSK and DFJLinK had slight differences in implementation before this. Nothing here that's too important to look at unless you want to, since all of the changes here were in DFJLinK already.
@@ -630,7 +739,408 @@ void DFJCOSK::build_J(std::vector<std::shared_ptr<Matrix>>& D, std::vector<std:: | |||
|
|||
} | |||
|
|||
void DFJCOSK::build_K(std::vector<std::shared_ptr<Matrix>>& D, std::vector<std::shared_ptr<Matrix>>& K) { | |||
// To follow this code, compare with figure 1 of DOI: 10.1063/1.476741 | |||
void CompositeJK::build_linK(std::vector<SharedMatrix>& D, std::vector<SharedMatrix>& K) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just adding LinK. No real changes to the LinK algorithm from DFJLinK.
@@ -71,10 +71,12 @@ JK::~JK() {} | |||
std::shared_ptr<JK> JK::build_JK(std::shared_ptr<BasisSet> primary, std::shared_ptr<BasisSet> auxiliary, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changes in this function are related to the CompositeJK front end - probably worth a look.
* | ||
* JK implementation using a direct density-fitted coulomb algorithm |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This block is just the removal of DFJCOSK.
*/ | ||
class PSI_API DFJLinK : public JK { | ||
class PSI_API CompositeJK : public JK { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The diff interpreted the CompositeJK implementation as a modification of DFJLinK.
The new CompositeJK class is essentially a union of the DFJLinK and DFJCOSK classes, with small changes to support the new front end.
All right, I went through the diff and tried to do some clarifications on the changes made. For other clarifications or more questions, please let me know. |
Also, here's what I get for the CI error:
This seems unrelated to the changes in this PR? |
psi4/src/psi4/libfock/jk.cc
Outdated
bool do_density_screen = options.get_str("SCREENING") == "DENSITY"; | ||
bool do_df_scf_guess = options.get_bool("DF_SCF_GUESS"); | ||
|
||
bool can_do_density_screen = (jk_type == "DIRECT" || jk_type == "LINK"); | ||
bool can_do_density_screen = (jk_type == "DIRECT" || jk_type == "DIRECTDFJ+LINK"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just checking that intend to filter on J+K, not just J here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is indeed intended behavior! Although it honestly might be better changed.
In the current version of the code, DFJCOSX fails the subsequent check using the can_do_density_screen variable, which happens when can_do_density_screen is set to false. This can be seen by looking at the original code before alteration here, where only the jk_types DIRECT and LINK result in can_do_density_screen=true, and not a jk_type of COSX. Practically, this means that SCF_TYPE=COSX throws an exception when SCREENING is set to DENSITY. I believe the reason the reason things were programmed this way, is because SCREENING=DENSITY does density screening from within TwoBodyAOInt, while DFJCOSX has its own density screening scheme implemented within itself.
The intentional filter on both J+K here is a continuation of the current behavior, where using the COSX algorithm in conjunction with SCREENING=DENSITY throws an exception.
@@ -48,7 +48,7 @@ info = psi4.DSYEV(0, 'V','U', n, A, n, D, W, 4*n) | |||
A.name = "Eigenvectors" | |||
|
|||
# Make first element of eigenvector positive if desired | |||
for i in range(1,n): | |||
for i in range(0,n): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fyi, there's never any need to hand edit or PR samples/
files. only include them if you've run the docs build and they appear "for free".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do remember you mentioning this previously, but thanks again! I did run the docs build as part of modifying the documentation regarding composite methods, but it's entirely possible I did a hand edit of this one, as well. My bad!
707c4f6
to
4cf2fcb
Compare
CI error with the recent updates:
Seems to be the CI acting goofy again. |
Further CI update - it seems the ddd-deriv test fails with this PR... but only on Windows. |
I think the ddd-deriv is a fluke -- we were seeing random Windows fails in Dec. After #2843 is merged, you can rebase to fix the ecos and give azure win another chance. |
bfba9b9
to
f72dbf5
Compare
9e3c509
to
bf3c605
Compare
Co-authored-by: Lori A. Burns <lori.burns@gmail.com>
Co-authored-by: Lori A. Burns <lori.burns@gmail.com>
Co-authored-by: Lori A. Burns <lori.burns@gmail.com>
7906b20
to
e705f4e
Compare
So before this gets merged in officially, I would like to regenerate the relevant reference output files, especially with the DirectDFJ->DFDirJ change. |
Description
This is it. It's here.
This PR is the initial implementation of the CompositeJK framework, the culmination of many, MANY of the JK-related PRs that are either in progress, or have been added to Psi4 already.
Many methods have been added to Psi4's JK class which enable utilization of algorithms that build J or K separately to improve performance. The ones in Psi4 as of now are the JK subclasses DFJLinK and DFJCOSK, which, between the two, contain integral-direct density-fitted J construction, the Linear Exchange method (LinK), and the Chain-of-Spheres Exchange method (COSX). Further ones, such as the Continuous Fast Multipole Method (CFMM), are planned for the future, and even more (e.g., J-Engine, Local DF methods) exist beyond the work done in Psi4 for the past year. The problem is, Psi4's JK class does not have a framework that truly supports the existence of these separate J and K algorithms. DFJLinK and DFJCOSK work for now, given the low number of separate J and K algorithms current in Psi4. But, as more separate J and K algorithms are added to Psi4, the number of JK subclasses will skyrocket, assuming every combination of separate J and separate K algorithm is stored in its own JK subclass. This will lead to a nasty problem with code duplication, not even discussing the pollution of the JK hierarchy that would result. Thus, for continued research into separate J and K construction algorithms, a proper framework for supporting such algorithms is paramount.
CompositeJK is that framework. CompositeJK is a single JK subclass which enables the combination and execution of any separate J and K build algorithm available in Psi4. CompositeJK can enable this without duplication of separate build algorithms across different subclasses. Additionally, CompositeJK is designed to enable simple expansion of itself, enabling the easy addition of new separate J and K builds to Psi4 with minimal programming issues.
The current PR is the first implementation of the CompositeJK framework into Psi4, and it has two components:
J_alg+K_alg
, where J_alg is the separate J build algorithm of choice, and K_alg is the separate K algorithm of choice. As of now, the two CompositeJK options available areDIRECTDFJ+LINK
representing the old DFJLinK class, andDIRECTDFJ+COSX
representing the old DFJCOSK class. The key here, is that the "+" delimiter in SCF_TYPE serves as a signal to Psi4 that a CompositeJK algorithm is being used.User API & Changelog headlines
Dev notes & details
To-do
Questions
Checklist
Status