-
Notifications
You must be signed in to change notification settings - Fork 440
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
SAPT(DFT) MO disk I/O optimization & Exchange-Dispersion scaling scheme update #2481
Conversation
This PR includes your work-in-progress implementation of automatic GRAC shifts, which is not ready for review. Please revise the PR to only include the code that is actually supposed to be part of the PR. |
Done. I've completely removed the automatic GRAC shift computations, because this could be non-trivial due to uncertain SCF converging issues, and I and Dr. Sherrill have agreed to postpone this feature until we find a better solution. |
psi4/src/psi4/lib3index/dfhelper.cc
Outdated
@@ -232,6 +232,7 @@ void DFHelper::AO_core() { | |||
required_core_size_ += 3 * nbf_ * nbf_ * Qshell_max_; | |||
|
|||
// a fraction of memory to use, do we want it as an option? | |||
// Set mem_frac to 0.5 when a direct metric contraction is needed (last part of DFHelper::transform()) to save memory for M and F |
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.
Why is this comment here? mem_frac
isn't even set 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.
I later decided that this should not be set. Removed the comment (not yet committed, planning to commit it along with clear_AO()
)
Ping me for review again when you have this passing tests. |
e1b1a8e
to
2fa911e
Compare
While I appreciate that tests are now passing, I still request changes.
|
I'm not quite sure what do they refer 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.
I want @CDSherrill to confirm that these changes to the SAPT parameters are meant to come in now.
The fact that you can change this parameter without breaking tests indicate that our SAPT(DFT) tests are not good enough.
@@ -2015,7 +2016,13 @@ void DFHelper::transform() { | |||
|
|||
// transformations complete, time for metric contractions |
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.
What is the transform function trying to accomplish? The function isn't properly documented, and I can't review the code without that.
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 transform function converts 3-index integrals with AO functions (pq|Q) to those with MO functions. The details of how they should be transformed are configured via other functions like add_space()
and add_transformation()
, and transform()
performs the actual transformation with the configurations defined above.
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.
partial review.
psi4/src/psi4/lib3index/dfhelper.h
Outdated
@@ -105,6 +105,9 @@ class PSI_API DFHelper { | |||
void set_AO_core(bool core) { AO_core_ = core; } | |||
bool get_AO_core() { return AO_core_; } | |||
|
|||
/// Switch flag for releasing AO (for SAPT(DFT)) |
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.
More guidance in the comment would be helpful. Why would you want it released or when would you want it preserved?
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 function transform()
consists of 2 steps:
- Transforms 3-index integrals with AO basis (pq|Q) to those with MO basis such as (ia|Q)
- Contract it with a power of metric (since it will be useful in some density fitting algorithms). Most common choice of power would be -1/2 and this contraction would yield (ia|Q)(Q|Q')^(-1/2).
In SAPT(DFT) hybrid dispersion, when in-core AO is turned on, the AO integral (pq|Q) is saved in the memory after step 1, but it is not needed in step 2, and releasing it allows much better blockings in step 2. (In fact what actually happened is that the memory limit was exceeded when trying to run large calculations before allowing the release of (pq|Q) after step 1)
However, it seems that when we release it, it will cause some other modules to fail, in particular Rob's FISAPT program for computing exchange-dispersion energy. This might be because in FISAPT (pq|Q) are still needed in step 2, but I have not figured out why this is the case. To avoid further complexity, I decided to release (pq|Q) only in SAPT(DFT) hybrid dispersion by using this switch flag.
I think we have manually set |
Leave those tests as they are, and create new tests for the fixed option. I also insist that you include a warning about those keyword changes in the documentation for SAPT(DFT), and in the output or logging file whenever SAPT(DFT) runs. This could cause users to get different numbers with the exact same input file. |
Created new test cases in the same input file, i.e. doing non-hybrid/hybrid+DISP/hybrid+FIXED in the same test file. I think this is the way to go since it is what we did when adding in the hybrid feature. Warning messages added in documentation and SAPT(DFT) output. |
Is this PR ready for another round of review? If so, please mark the "ready for review" option on the first post here. That's how core developers can tell if a PR is ready or not. |
cbdb50a
to
723f820
Compare
Ready for review now, thanks. |
I've discussed this PR with Yi. This is just to confirm that we want to change the default exch-disp scaling scheme, as the new one appears to be more reliable. Apparently the previous test case didn't break because the exch-disp scheme was manually selected in the test case, rather than utilizing the (updated) default. |
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.
We have no test coverage on the DFT_DO_DHF
keyword, and this PR changes its behavior.
I'm going to request a separate PR that adds this test coverage. That PR must be merged in first, so that we can be sure this PR is not changing the results from that keyword.
@@ -1459,7 +1459,8 @@ void Matrix::axpy(double a, SharedMatrix X) { | |||
} | |||
for (int h = 0; h < nirrep_; h++) { | |||
size_t size = colspi_[h ^ symmetry()] * (size_t)rowspi_[h]; | |||
if (size != (X->rowspi()[h] * X->colspi()[h ^ X->symmetry()])) { | |||
size_t size_X = X->colspi()[h ^ X->symmetry()] * (size_t)X->rowspi()[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.
Why is this change necessary? Was there some issue with type casting?
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.
Yes. If I remembered it correctly this some how broke down when large vectors were passed in.
Is a few new test cases everything that new PR should have, or should I cherry-pick the changes related to the |
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.
Thanks for the updates, Yi. Here's a few suggestions on notating the defaults changes.
The new PR should only have test changes. The point of me asking for this is to make sure that this PR does not change the |
Hi Yi, Thanks for your work and the comments. I've been discussing some with Jonathon, and I think if you could do the below, that will clarify all the scaling factor and testing aspects of the PR, so we can move on to the I/O optimization and routing logic parts. Please let me know of any concerns.
|
Added the !dHF cases in Added the scaling scheme/factor output lines in the output file. |
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.
Thanks! here's a couple more GH suggestions on the exch-disp.
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>
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>
Co-authored-by: Lori A. Burns <lori.burns@gmail.com>
Co-authored-by: Lori A. Burns <lori.burns@gmail.com>
45a5f2d
to
a53c5e3
Compare
Apologies, please add |
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.
lgtm, thanks for all the improvements!
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.
[Original comment removed after Lori clarified my confusion.]
|
||
.. warning:: Since Nov 2022, the defaults of options |sapt__sapt_dft_exch_disp_scale_scheme| and |sapt__sapt_dft_exch_disp_fixed_scale| | ||
have been changed. Before, the former defaulted to ``FIXED`` with Hesselmann value of 0.686 for the latter. Now, the former defaults to ``DISP`` and should you instead select ``FIXED``, the default for the latter is the Xie value of 0.770. This might cause |
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.
Link to the Xie paper the first time you mention it. (The first mention may be in one of my suggestions.)
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.
Added the paper. Rephrased a bit on "Hesselmann appraoch" and "Xie approach"; I would call them "Podeszwa approach" and "Hesselmann approach" respectively instead (see references).
While I did not give this a very careful pass, I've given it enough of a pass to merge it in once the previous two comments I made are addressed. @hokru, did you want another pass of this? |
Co-authored-by: Jonathon Misiewicz <jonathon.paul.misiewicz@emory.edu>
Description
Optimizes the integral transformation step of SAPT(DFT), in which the transformed MO needs to be written to the disk. The
STORE
scheme insrc/psi4/lib3index/dfhelper.cc
has problem in writing blocks of integrals efficiently, and this PR changes it todirect_iaQ
to optimize the writing process. Also fixed a few memory related bugs and modified a few timer tags.The deafult scaling scheme of SAPT(DFT) exchange-dispersion energy is now changed from
DISP
toFIXED
. It scales the uncoupled Exch-Disp2 by 0.769848. Deeper details are discussed in the paper [Y. Xie, D. G. A. Smith, and C. D. Sherrill, J. Chem. Phys. 157, 024801 (2022)].The SAPT(DFT) procedure is also optimized for the case that
SAPT_DFT_FUNCTIONAL = HF
, i.e. running SAPT0 with the SAPT(DFT) driver, to avoid redundant SCF calculations.Todos
Questions
Checklist
Status