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

dfh: fix direct_iaq memory estimate in AO_core module #1100

Merged
merged 1 commit into from
Jul 23, 2018

Conversation

obrien951
Copy link
Contributor

Todos

Notable points that this PR has either accomplished or will accomplish.

  • Developer Interest
    • fix direct_iaq memory estimate in AO_core module

Checklist

Status

  • Ready for review
  • Ready for merge

@loriab loriab added this to the Psi4 1.3 milestone Jul 20, 2018
if(direct_iaQ_) {
// the direct_iaQ method does not use sparse storage
// if do_wK added to code, the following will need to be changed to match
required = naux_ * nao_ * nao_ ;
Copy link
Member

Choose a reason for hiding this comment

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

@schiebermc Can you review this?

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think this changes anything.
Having dense integrals implies that big_skips_[nao_] == naux_ * nao_ * nao_.
Is there a reason this was added in? did something break?

Copy link
Member

Choose a reason for hiding this comment

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

@CDSherrill can explain more fully, but something did break. In a direct_iaq (fisapt) test with a certain amount of memory, it was passing through this memory estimator cleanly, then at AO_core transform, it was failing that memory estimator and throwing an error. With this fix in place, it noticed memory was insufficient for AO_core so switched to disk and completed correctly. Perhaps you can post the triggering input file, @obrien951.

Copy link
Contributor

@schiebermc schiebermc Jul 20, 2018

Choose a reason for hiding this comment

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

Aha, upon further inspection this is probably the case. I thought that I had switched cutoff_ = 0.0 when direct_iaQ_, but this appears to not be the case. Easily confirmable if the mask sparsity printed to the output is not 0.0%. Therefore big_skips_[nao_] represents the total number of doubles for the sparse form, even when direct_iaQ_.

Copy link
Contributor

Choose a reason for hiding this comment

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

In fact line 484 uses the correct memory estimate (direct from naux_, nao_). I think the issue was that AO_core() was added during the JK building push, so I did not consider the dense integrals from direct_iaQ. Probably the discrepancy between this and lines 605-607 causes the throw. I approve of the changes.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for checking this, @schiebermc

Copy link
Member

@dgasmith dgasmith left a comment

Choose a reason for hiding this comment

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

LGTM

@loriab loriab merged commit d6a9813 into psi4:master Jul 23, 2018
@dgasmith dgasmith mentioned this pull request Dec 6, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants