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

conventional REMP2 and OREMP2 #2653

Merged
merged 1 commit into from
Aug 9, 2022
Merged

conventional REMP2 and OREMP2 #2653

merged 1 commit into from
Aug 9, 2022

Conversation

loriab
Copy link
Member

@loriab loriab commented Aug 1, 2022

Description

REMP methods added to occ and attendant testing and routing.

This is PR 1/? in the mega-dfocc-remp series.

PR Background (not of general interest)

There was https://github.com/loriab/psi4/tree/dfocc2 by @bozkaya and @yavuzalagoz that added to dfocc (1) non-oo uhf ccsd-level E & G, (2) oo ccd-level E & G, and (3) fno everything. Then, there was #2354 that added remp2 E & G to occ and dfocc and made extensive improvements to dfocc in the matter of combined DIIS (vital for convergence) and int overflows. All based on pre-DIIS overhaul #2369 . To untangle this, I started from master, cherry-picked the dfocc2 commits, cherry-picked the #2354 commits, modernized the DIIS calls, then added the neglected stdsuite testing and attendant qcvar fixups for existing occ/dfocc methods and new methods remp2 and categories (1) and (2) from dfocc2 branch. That mega-changeset lives at #2633 for now. This PR breaks off the occ portion for review.

Todos

  • @behnle added REMP2 and OREMP2 hybrid perturbation theories (https://doi.org/10.1063/1.5086168) with conventional integrals added to occ module, as originally proposed in [Feature addition] Add the REMP and OO-REMP hybrid perturbation theories to OCC/DFOCC #2354
  • note that this uses a specialty QCEngine for reference values. stdsuite testing remp2, oo, dfcc MolSSI/QCEngine#375 It'll build by itself. I'll need to mint a QCEngine patch release before building conda packages with this PR in master.
  • docs-wise, I picked off a bit and included it in this PR. on the whole, let's defer docs edits until the dfocc PR
  • reworked the run_occ* driver functions a bit so that method defs are localized and there's less risk of missing an internal setting when adding new methods.
  • added stdsuite testing for new methods remp2 energy and oremp2 energy and gradient. added stdsuite testing for existing E & G for omp2, omp2.5, omp3, oremp2, olccd and filled in some gaps for ccsd, ccsd(t), a-ccsd(t). all conventional ints. fixed up some mis-set byproducts uncovered by stdsuite testing: OO ROHF was printing and setting wrong plain MP2 energies, OMP3 & OMP2.5 wasn't setting right Wfn.energy_, stop setting ROHF MP3 and MP2.5 out of caution.
  • tightened the min rms_mograd_convergence from 6.0 to 6.5 so that stdsuite (3 mol/basis sets) could reliably compute energies, gradients, and findif gradients to 1e-6.

Questions

  • sometimes the non-OO value (e.g., MP2, LCCD) is available as an early byproduct of the OO calc (e.g., OMP2, OLCCD). this wasn't the case with REMP, and some QCVariables that stored a purported non-OO REMP had to be removed. @behnle, please confirm that this sounds right.
  • @behnle, sorry for the long delay and scrambling of your PR. please feel free to comment and PR to this to modify.
  • @bozkaya, this adds pretty cleanly onto your occ code, but do look it over if you want.

Checklist

Status

  • Ready for review
  • Ready for merge

@loriab loriab added enhancement feature Extends an existing Psi feature or develops a new one. occ For issues involving OCC, orbital-optimization and more. labels Aug 1, 2022
@loriab loriab added this to the Psi4 1.7 milestone Aug 1, 2022
@behnle
Copy link
Contributor

behnle commented Aug 1, 2022

Excellent, thanks for your tremendous effort.
Given that my test cases were not modified and still seem to work, i do not have any complaints.

  • sometimes the non-OO value (e.g., MP2, LCCD) is available as an early byproduct of the OO calc (e.g., OMP2, OLCCD). this wasn't the case with REMP, and some QCVariables that stored a purported non-OO REMP had to be removed.

MP2 should also be available from the guess. But it is of course better to not print/store something than to print something wrong. I might be mistaken, but given that occ does coupled DIIS for amplitudes and orbitals, the canonical LCCD enery should not be available from an OLCCD calculation (lccd is never iterated on canonical orbitals). The same holds for REMP2. So yes, if there are variables pretending to be canonical results from an orbital-optimized calculation, these should probably be removed.
No problem, it was mostly my fault to put way too many changes into a single pull request based on an ancient master branch.

@JonathonMisiewicz
Copy link
Contributor

  • sometimes the non-OO value (e.g., MP2, LCCD) is available as an early byproduct of the OO calc (e.g., OMP2, OLCCD). this wasn't the case with REMP, and some QCVariables that stored a purported non-OO REMP had to be removed.

MP2 should also be available from the guess. But it is of course better to not print/store something than to print something wrong. I might be mistaken, but given that occ does coupled DIIS for amplitudes and orbitals, the canonical LCCD enery should not be available from an OLCCD calculation (lccd is never iterated on canonical orbitals). The same holds for REMP2. So yes, if there are variables pretending to be canonical results from an orbital-optimized calculation, these should probably be removed. No problem, it was mostly my fault to put way too many changes into a single pull request based on an ancient master branch.

This is completely correct. The lone OLCCD algorithm in occ does not compute LCCD with the input orbitals.

@loriab
Copy link
Member Author

loriab commented Aug 1, 2022

sometimes the non-OO value (e.g., MP2, LCCD) is available as an early byproduct of the OO calc (e.g., OMP2, OLCCD). this wasn't the case with REMP, and some QCVariables that stored a purported non-OO REMP had to be removed.

MP2 should also be available from the guess. But it is of course better to not print/store something than to print something wrong. I might be mistaken, but given that occ does coupled DIIS for amplitudes and orbitals, the canonical LCCD enery should not be available from an OLCCD calculation (lccd is never iterated on canonical orbitals). The same holds for REMP2. So yes, if there are variables pretending to be canonical results from an orbital-optimized calculation, these should probably be removed. No problem, it was mostly my fault to put way too many changes into a single pull request based on an ancient master branch.

This is completely correct. The lone OLCCD algorithm in occ does not compute LCCD with the input orbitals.

Great, thanks. oremp2 is in good shape, then, and I'll add negative assertions for olccd. Here's the summary of what gets checked (details are at QCEngine): https://github.com/psi4/psi4/pull/2653/files#diff-e2cf14f98c8e885f5abc7385ca737bfeba9f2f62caead630129e6d7cd9678e71R193-R203

Copy link
Contributor

@JonathonMisiewicz JonathonMisiewicz left a comment

Choose a reason for hiding this comment

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

Overall, I'm happy with this. Huge thanks to both @loriab and @behnle.

.github/workflows/docs-pr.yml Show resolved Hide resolved
doc/sphinxman/source/glossary_psivariables.rst Outdated Show resolved Hide resolved
doc/sphinxman/source/glossary_psivariables.rst Outdated Show resolved Hide resolved
doc/sphinxman/source/occ.rst Outdated Show resolved Hide resolved
doc/sphinxman/source/occ.rst Outdated Show resolved Hide resolved
psi4/src/psi4/occ/cepa_iterations.cc Outdated Show resolved Hide resolved
Comment on lines +195 to +198
t2_amps_remp(); // <- the only actual modification compared to regular CEPA(0)/D
timer_off("T2");
timer_on("CEPA Energy");
cepa_energy();
cepa_diis(t2_diis); // <- CEPA diis can be reused without modifications
cepa_chemist();
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a good rationale for renaming functions, so that things labeled cepa are actually cepa only. I'd be thrilled if we could condense this and the CEPA code into a single function that just passes a flag around.

Copy link
Member Author

Choose a reason for hiding this comment

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

for the future ...

psi4/src/psi4/occ/coord_grad.cc Show resolved Hide resolved
psi4/src/psi4/occ/manager.cc Show resolved Hide resolved
psi4/src/psi4/occ/manager.cc Show resolved Hide resolved
@loriab
Copy link
Member Author

loriab commented Aug 2, 2022

@behnle, were there any changes from the review that you wanted to PR into this PR? Otherwise, I'll force push my accumulated responses.

@behnle
Copy link
Contributor

behnle commented Aug 2, 2022

As far as i can see, this is fine as is.

@loriab
Copy link
Member Author

loriab commented Aug 3, 2022

Jonathon's comments all addressed.

Copy link
Contributor

@JonathonMisiewicz JonathonMisiewicz left a comment

Choose a reason for hiding this comment

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

🍺

@@ -107,6 +109,8 @@ def runner_asserter(inp, subject, method, basis, tnm):
# _recorder(qcprog, qc_module_in, driver, method, reference, fcae, scf_type, corl_type, "error", "nyi: " + reason)
return

psi4.set_output_file("asdf")
Copy link
Member

Choose a reason for hiding this comment

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

"asdf" intentional or placeholder?

Copy link
Member Author

Choose a reason for hiding this comment

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

Right now it's a hack solution to a mystery. Each job is supposed to produce a .out and a .log file. The log file shows up, but the out file doesn't even though I cancel deletions in conftest.py. If I change the base to asdf, both files show up, so I can debug. So, intentional but temporary.

global_dpd_->buf4_init(&Taa, PSIF_OCC_DPD, 0, ID("[O,O]"), ID("[V,V]"), ID("[O,O]"), ID("[V,V]"), 0,
"T2 <OO|VV>");
global_dpd_->buf4_init(&Tbb, PSIF_OCC_DPD, 0, ID("[o,o]"), ID("[v,v]"), ID("[o,o]"), ID("[v,v]"), 0,
"T2 <oo|vv>");
global_dpd_->buf4_init(&Tab, PSIF_OCC_DPD, 0, ID("[O,o]"), ID("[V,v]"), ID("[O,o]"), ID("[V,v]"), 0,
"T2 <Oo|Vv>");
t2_diis = DIISManager(maxdiis_, "CEPA DIIS T2 Amps", DIISManager::RemovalPolicy::LargestError, DIISManager::StoragePolicy::InCore);
Copy link
Member

Choose a reason for hiding this comment

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

Is there a reason for the different the StoragePolicy's for RHF and UHF?

Copy link
Contributor

Choose a reason for hiding this comment

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

No... Should probably be both OnDisk. I also propose that the initialization of t2_diis is taken out of the

if (reference_ ==  "RESTRICTED") {
...
}
else if (reference_ == "UNRESTRICTED") {
...
}

block. It seems redundant to me.

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 think the different policies is what Ugur had. But makes sense to unify them. I'll do it in PR 3, though, which will go up today.

Copy link
Contributor

Choose a reason for hiding this comment

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

For context, during my previous changes in the occ module, I unified them as behnle suggested. This was an unintended behavior change. Lori is changing back to how Ugur had it before my interventions.

@JonathonMisiewicz
Copy link
Contributor

Three cheers for three approvals. I'm deferring merge to Lori on a PR like this.

@loriab loriab merged commit d3d403a into psi4:master Aug 9, 2022
@loriab loriab deleted the occ_only branch August 9, 2022 16:23
@behnle
Copy link
Contributor

behnle commented Aug 9, 2022

Thank you all very mch guys, if i ever meet you, i definitely owe you a 🍺 .

@loriab
Copy link
Member Author

loriab commented Aug 9, 2022

Thank you all very mch guys, if i ever meet you, i definitely owe you a 🍺 .

You're very welcome. Thanks for (1) the new methods, (2) the additional motivation to address the technical debt in occ/dfocc, and (3) your fixes to dfocc convergence to make (2) possible :-) . I owe you a return 🍺 and certainly a Psi mug. We'll have to coordinate addresses/conferences on the latter.

@loriab loriab mentioned this pull request Sep 12, 2022
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement feature Extends an existing Psi feature or develops a new one. occ For issues involving OCC, orbital-optimization and more.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants