-
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
capabilities maintenance #2731
capabilities maintenance #2731
Conversation
Is there anything in particular you wanted feedback on, or did you just want to see if this passed tests? |
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. |
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've gone through a first pass. Please check to ensure all newly created functions are documented.
I can re-review when this passes tests. |
Tests failing because a typing Any import missing. I was going to fix when other changes accumulate. |
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." |
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.
After discussion with Lori, rubber stamp approval.
I recommend, per usual, that future PRs be more piecemeal.
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've quickly scrolled through the changes and we've discussed the opt-in qc module changes, so ok from my side 👍🏻
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 rubber stamp than review.
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
set qc_module mrcc
rather than "mr" prefix onto methodset 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:
Other changes to docs contents:
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 GAMESSospt=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 withmethod_algorithm_type(mtd).keyword
andmethod_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,
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 themethod_algorithm_type
fnrun_fnodfcc
,run_fnocc
,run_cepa
were simplified by using themethod_algorithm_type
fn and the "director" dictionary syntaxThe 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.
standard_suite_ref_local.py
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.htmlfixed
energy("psimrcc_scf")
to use regular SCF as ref for PSIMRCC and added a testrehabilitated
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
Checklist
Status