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

capabilities maintenance #2731

Merged
merged 9 commits into from
Oct 21, 2022
Merged

capabilities maintenance #2731

merged 9 commits into from
Oct 21, 2022

Conversation

loriab
Copy link
Member

@loriab loriab commented Sep 29, 2022

Description

The common thread of this PR is be more explicit in what calculations Psi4 can do and conveying that to the user at runtime and through the docs. In practice, this involves expanding the coverage of stdsuite, using those results to autogenerate docs tables, reconciling hand-generated docs tables, referring to all in runtime errors, and cleaning stuff up along the way.

User API & Changelog headlines

  • MRCC now called with set qc_module mrcc rather than "mr" prefix onto method
  • Many NYI messages, whether from ManagedMethodError, MissingMethodError, or plain ValidationError from run_* fns have changed their wording and added content. On the whole, this should be helpful, but if you're searching for particular phrasing, you'll likely need to adjust regexes.
  • New tables in the docs with details on accessible methods.
  • Arbitrary-order MPn no longer runable with ROHF. Arbitrary-order ZAPTn no longer runable with RHF.
  • Running DF through CCENERGY (experimental, expert only) may now require set qc_module ccenergy.

Dev notes & details

  • running stdsuite writes a storage file, and a new document_capabilities script turns the storage file into various tables that newly (1) include all-electron/frozen-core info (notated "aefc" in places), (2) show default modules and default e.g., mp2_type, (3) cover most single-ref ground-state total-energy methods, (4) are proven to honor return val and qcvar contracts. details of how and why this works are at psi4/share/psi4/scripts/merge_stdsuite.py . If you want to suggest changes to notation, keep in mind that main-body cell contents must be expressible in a single unicode character width, otherwise the table gets far too wide; layering allowed. Several tables are produced:

    • an all-methods, one row per method table for the front page: introduction.rst#capabilities . this table complements the comprehensive hand-written one. summary
    • an all-methods, one row per module per method table for the "Alternate Implementations" page: details now contains info on all stdsuite-tested methods, not just those implemented in more than one module
    • various tables of specific-module-methods, one row per method for the individual module pages:
      • ccenergy pre-approved by TDC in BCCD and CC docs #2708
      • fnocc this replaces a hand-generated table, but a broader hand-generated table is also present for (1) method names and (2) FNO functionality (not yet covered by stdsuite). the fnocc table is energy (E) only, whereas most are E/G
      • scf the only E/G/H table. this includes HF and some representative DFTs: LSDA, GGA, Hybrid, LRC, DH
      • occ_nonoo replaces a hand-generated table, but a broader hand-gen table is also present for (1) method names and (2) in anticipation of FNO.
      • occ_oo replaces a hand-generated table, but a broader hand-gen table is also present for (1) method names. closes Document the relationship between OCC/DFOCC and frozen core #1863
      • dfmp2 pretty simple, so why not
  • Other changes to docs contents:

    • instead of detailed capabilities getting lost under notes in the TOC, moving it to a separate page, to be followed by notes.
    • the main hand-generated capabilities table in introduction.rst tried to do a lot in specifying allowed refs, types, dertypes, and parallelism, including by specifying exceptions in footnotes. This discouraged updating since it was hard to know what was right w/o extensive testing and/or it was easier to glide over details. The hand-gen table has been replaced to be more general listing maximum capabilities, the parallelism column has been removed, and the complementary auto-gen summary table has been added
    • the main hand-generated capabilities table in introduction.rst and the docstring methods table in driver.py (shows up in https://psicode.org/psi4manual/master/energy.html etc.) have been reconciled with the current state of the procedures table. Devs are now admonished to update all three concurrently :-)
  • Some investigation and searching of CDS's memory and comparison to GAMESS concluded that for arbitrary-order MPn and ZAPTn through DETCI that the written docs were right (MPn for RHF only and ZAPTn for ROHF only) and what was allowed was wrong (both for both). This has been reworked to match the written docs and various UpgradeHelpers have been positioned. Some tests had to change. Note that ZAPTn reduces to MPn for RHF, and DETCI was producing that correctly. However, DETCI MPn for ROHF wasn't producing anything we had confidence in, so it's good to get that disabled. Ugur's ROHF MP2 matches GAMESS ospt=rmp and DETCI's ROHF ZAPT2 matches GAMESS ospt=zapt, so I think this puts detci rohf mp2 #311 to rest.

  • As reported in Some MRCC-related tests fail #2634, MRCC wasn't playing nicely with distributed driver due to the two-stage call-with-mrmtd then intercept-and-replace-mtd-with-dict. This scheme is replaced by an ordinary method call with MRCC backend indicated by set qc_program mrcc, just like intra-psi modules. Psi4 won't just default to MRCC if available; it must be specified. An UpgradeHelper has been deployed. The MRCC definition dictionary has been moved from driver_util.py (in a fn) to a raw dict in procrouting/proc_data.py. Collection of qcvars has been maximized. closes Some MRCC-related tests fail #2634. replaces and closes mrcc fix after ddd #2638 EDIT: after discussion below, MRCC will be defaulted to if available. Practically, the only confusion should arise for a-ccsd(t) where Psi4 has the method sometimes (rhf) but not others (uhf/rohf). Methods like ccsd will always default to ccenergy, and methods like ccsdt will always go to mrcc.

  • Like to the MRCC case above, the arbitrary-order methods mp_n_, zapt_n_, ci_n_ that were intercepted and then transferred around as a method (e.g., "mp") and a level (kwarg "level=5") were requiring extra complication to work with the distributed driver. Now all such methods and a reasonable number of levels are added explicitly to the procedures table and splitting/parsing happen at run_detci.

  • the scf_type, mp2_type, mp_type, ci_type, cc_type keywords were a good idea in that they allow different defaults for different levels of theory and are fairly easy to guess and uniform to use. One flaw is that you don't know programmatically what controls what method -- it's all hard-coded, sometimes in more than one place, in proc.py . Now there's a method:type association dict in procrouting/proc_data.py . There's also a little function there serving up the info, so you can get out the keyword and current value with method_algorithm_type(mtd).keyword and method_algorithm_type(mtd).now, respectively. This helps clean up proc.py and allows the controlling keyword to be linked for each method in the generated tables.

  • in proc.py,

    • select_* functions have been made more boilerplate and elsewhere-mentioned DETCI and MRCC changes integrated in.
    • CCD is given select_ccd* functions to raise a sensible error message since the default (cc_type=CONV) isn't implemented.
    • CC2 and CC3 are given select_* functions since now CCENERGY and MRCC cover them. This led to controversy (unresolved) over whether DF-CC2 is RTG, see DF in CCENERGY module #2710. A result is that you need to set qc_module ccenergy explicitly to use DF for CC2 -- a test case edit was needed.
    • run_dfocc, run_dfocc_gradient fns were simplified by using the method_algorithm_type fn
    • run_fnodfcc, run_fnocc, run_cepa were simplified by using the method_algorithm_type fn and the "director" dictionary syntax
    • some impossibilities fended off rather than being silently ignored: non-df-mp2 in dhdft, non-conv detci
  • The involved which-do-I-run--energy-gradient-or-hessian logic in negotiate_derivative_type and friends in driver_util.py does a great job at its task, but its error messages have been wanting, being hampered by being generated by exceptions. In particular, they give no clue how close you are to a working method -- do you need to tweak aefc or uhf/rohf or is this method nowhere in psi or have you misspelled it? Now, the ManagedMethodsError exception has been reworked to return current conditions data (to be caught and reformatted) and the error message itself (usually not seen directly) has been reworked to provide more conditions and a link to the table row in the docs where one can see what would run. Back at driver_util.py, these errors are reformatted according to whether managed or plain method, whether deriv isn't available at all or just not demanded deriv, etc. Whenever possible, try to provide conditions that didn't run and a docs link. Some examples of before and after at https://github.com/psi4/psi4/pull/2731/files#diff-d6e974accd9a58a9993b3babac9fab5c85099ba6072de01c9a6bcc4ad430dc56L22-R82 .

  • Added mp4(sdq), mp4, zapt2, cisd, qcisd, qcisd(t), fci, cepa(1), cepa(3), acpf, aqcc, ccd, bccd, bccd(t), cc2, cc3, and some representative DFTs (svwn, pbe, b3lyp, wb97x, b2plyp) to standard suite testing to nail down their capabilities. Added some advice to the stdsuite so that others can perhaps edit it.

    • all but cepa(1), cepa(3), acpf, aqcc, svwn, and wb97x are verified against external programs (for qcdb's and qcengine's good, not because I distrusted longstanding Psi4 implementations), usually cfour, nwchem, gamess
    • ROHF DETCI doesn't match gamess/cfour for CISD or FCI, but doubtless that's implementation choice that I haven't identified. Anyway, that's why those are separate entries (away from qcng) in standard_suite_ref_local.py
    • In filling out the capabilities tables for Hessians, I tried to collect CD Hessians for HF and SVWN by FD of energies. They failed the per-element CD result ~= CONV result check. Not surprising since CD thresh is set at 1e-4. I didn't do anything about the failure (ref values deleted and tests are not active), so this is just to bring up that CD threshold is untuned for dertype or E_/D_/R_CONVERGENCE.
    • refuted first two thirds of a prominent footnote: DFT gradients only implemented for SCF type DF. LRC-DFT gradients not implemented yet. DH-DFT gradients not implemented.
  • upgraded stdsuite runner to catch up with qcdb and qcengine last year by (1) implementing per-dertype and abs/rel comparison checks (nothing is actually loosened) and (2) dropping a dict summary of each test to a record file.

  • arranged for available td-{dft} calls to show up in the energy table, https://psicode.org/psi4manual/master/energy.html

  • fixed energy("psimrcc_scf") to use regular SCF as ref for PSIMRCC and added a test

  • rehabilitated energy("qchf") (which never had its initialism expanded in the whole of the codebase, btw) so it runs. added a test. it probably ought to be better integrated with regular SCF module. Regular SCF has a qchf option that is unused.

  • adjusted some qcvars in FNOCC to collect more components from cepa-like methods and to collect different perturbative triples for qcisd.

  • collected qcvars correctly in DFOCC for CCD and QCHF. (It was harvesting the CCD method that tipped me into the rabbit-hole that became this PR.)

  • TODO pytest -m quick is still friendly to run (CI finished in reasonable time), but the additions to stdsuite have make ordinary full pytests expensive. I need to manipulate default marks so folks don't inadvertently start hours of tests. pytest -v ../tests/pytests/test_standard_suite.py -m "not noci" -n auto --durations 100 --durations-min 60.0 runs in 25 minutes on a 20-core machine with no individual test over 4m. That's not reasonable for a laptop. Blame the slow-to-converge oo methods, and it's going to get worse with occd and friends.

  • TODO cc2, cc3, bccd, cepa, dhdft, sdsc separation, zapt. format MolSSI/QCEngine#376 and a new version of QCEngine are a prereq.

  • stdsuite: sdsc and new methods from psi4 work Summer 2022 qcdb/qcdb#57 is associated QCDB PR (psi4 PR is a prereq to it)

  • TODO full stdsuite in the presence of MRCC will have a couple faults of the "not NYI" variety after the MRCC defaulting logic switch from Q. I haven't decided how to remedy this, but it's unlikely to be hit, so I'll defer.

Questions

  • My version of Psi4+MRCC doesn't run CC2. Anyone else see this? (Just curious, not really needed for PR.)
  • I don't think the original module authors will find issues, but if you want to look over pertinent docs and capabilities tables, please do so.
  • [ADDED from Remove built-in ADC module #2737 For methods (or methods in certain circumstances, say reference or conv/df) only available through an external add-on, do we want those opt-in? That is, certainly the external must be (1) installed and detectable. But do we also want to (2) require the user to set qc_module=mrcc|adcc|chemps2 ? CheMPS2 has a long history of not requiring (2). ADCC has a shorter history of being the preferred backend and automatic choice, if present. @maxscheurer prefers not requiring (2). In this PR, I just switched MRCC syntax to yes require (2). That was in keeping with the user opt-ing in via energy("mrccsd"). I can go either way, and I guess I'm now leaning toward not requiring (2) and adjusting MRCC accordingly. But it seems like something to discuss and settle on a consistent treatment. EDIT: ok, MRCC can be a default. above section edited.

Checklist

Status

  • Ready for review
  • Ready for merge

@JonathonMisiewicz
Copy link
Contributor

Is there anything in particular you wanted feedback on, or did you just want to see if this passed tests?

@loriab loriab added this to the Psi4 1.7 milestone Sep 29, 2022
@loriab loriab added documentation testing compatibility external-interface For issues about interfaces with external programs: ADCC, CheMPS2, GDMA, MRCC... fnocc For issues with the FNOCC module. cleanup For issues where the goal is to make Psi4 a little cleaner. dfocc For issues in the DFOCC module. driver For issues that are specifically about Psi's driver. detci For issues with the DETCI module. labels Sep 29, 2022
@TiborGY
Copy link
Contributor

TiborGY commented Sep 30, 2022

Thanks for the extensive docs/UI pass. If you need some additional reference data for a particular method that Molpro or Orca 5 has, I could do that.

@loriab
Copy link
Member Author

loriab commented Sep 30, 2022

Thanks for the extensive docs/UI pass. If you need some additional reference data for a particular method that Molpro or Orca 5 has, I could do that.

Thank you, @TiborGY, I'll keep that in mind.

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.

I've gone through a first pass. Please check to ensure all newly created functions are documented.

doc/sphinxman/CMakeLists.txt Show resolved Hide resolved
doc/sphinxman/source/add_tests.rst Show resolved Hide resolved
tests/pytests/standard_suite_ref_local.py Outdated Show resolved Hide resolved
tests/pytests/conftest.py Outdated Show resolved Hide resolved
tests/pytests/standard_suite_runner.py Show resolved Hide resolved
psi4/driver/p4util/exceptions.py Show resolved Hide resolved
doc/sphinxman/document_tests.pl Show resolved Hide resolved
psi4/driver/p4util/exceptions.py Show resolved Hide resolved
doc/sphinxman/document_capabilities.py Outdated Show resolved Hide resolved
doc/sphinxman/document_capabilities.py Show resolved Hide resolved
@JonathonMisiewicz
Copy link
Contributor

I can re-review when this passes tests.

@loriab
Copy link
Member Author

loriab commented Oct 7, 2022

Tests failing because a typing Any import missing. I was going to fix when other changes accumulate.

@JonathonMisiewicz
Copy link
Contributor

But do we also want to (2) require the user to set qc_module=mrcc|adcc|chemps2 ?

If you mean require it to use the add-on only method, I would say no. I don't understand the rationale for an answer of "yes."

@JonathonMisiewicz JonathonMisiewicz mentioned this pull request Oct 10, 2022
4 tasks
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.

After discussion with Lori, rubber stamp approval.

I recommend, per usual, that future PRs be more piecemeal.

Copy link
Contributor

@maxscheurer maxscheurer left a comment

Choose a reason for hiding this comment

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

I've quickly scrolled through the changes and we've discussed the opt-in qc module changes, so ok from my side 👍🏻

Copy link
Member

@hokru hokru left a comment

Choose a reason for hiding this comment

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

More rubber stamp than review.

@loriab loriab merged commit c5ae134 into psi4:master Oct 21, 2022
@loriab loriab deleted the captab_rb2 branch October 21, 2022 03:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cleanup For issues where the goal is to make Psi4 a little cleaner. compatibility detci For issues with the DETCI module. dfocc For issues in the DFOCC module. documentation driver For issues that are specifically about Psi's driver. external-interface For issues about interfaces with external programs: ADCC, CheMPS2, GDMA, MRCC... fnocc For issues with the FNOCC module. testing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Some MRCC-related tests fail Document the relationship between OCC/DFOCC and frozen core
5 participants