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

Refactor Recursive Driver Into Distributed Driver #1351

Open
wants to merge 157 commits into
base: master
Choose a base branch
from

Conversation

@alenaizan
Copy link
Contributor

@alenaizan alenaizan commented Nov 10, 2018

Needs Py36

Description

Starting PR. Change the Psi4 driver from being recursive to being based on compute classes that have planning, computing and querying functions. Then, integrate the new driver with the QCArchive project for parallelizing nbody, CBS and findif.

Todos

Notable points (developer or user-interest) that this PR has or will accomplish.

  • Create compute classes for nbody, CBS and findif
  • Integrate QCArchive

Release Notes

  • msgpack-python added as req'd dependency. this keeps numpy arrays serialized when communicating in schema
  • python 3.8 requires pint 0.10
  • add a logging file, presently file.log that currently is continuously appended
  • any fns mirroring sherrill_gold_standard or allen_focal_point need to be reformatted and registered with register_composite_function
  • extrapolation functions for composite need to be registered with register_xtpl_function
  • CBS and xtpl functions need to be called as strings, not objects. So energy('cbs', scf_wfn='scf', scf_basis='cc-pV[DTQ]Z', scf_scheme='scf_xtpl_helgaker_3') not energy(cbs, scf_wfn='scf', scf_basis='cc-pV[DTQ]Z', scf_scheme=scf_xtpl_helgaker_3).
  • no longer always evaluating gradient before Hessian to test safety of projecting rotations. Instead assuming unsafe. Can pass ref_gradient array to test on or set findif fd_project T/F explcitly to control.
  • psi4.QMMM() object replaced by embedding_charges kwarg in Bohr. See extern examples for details.
  • json_ret = psi4.schema_wrapper.run_qcschema(json_input) not json_ret = psi4.json_wrapper.run_json(json_input). json_ret is now an object, so . access, not dictionary.
  • CBS wrapper issues a clean() btwn calcs. Plain string modelchem calcs like energy('hf/cc-pvdz') were getting caught and also being cleaned, meaning their behavior was slightly different than set basis cc-pvdz \n energy('hf'). This is no longer happening, so for occasional string modelchem calcs, you may need to add a clean in the input.
  • extern object units are bohr, not whatever the Molecule was.
  • psi4 --module returns the path for loading via import psi4. opposite of psi4.executable
  • psi4.set_output_file now wraps psi4.core.set_output_file and additionally sets up a truncating python logging file with .log extension at the same time as the .out file.
  • Besides the input.dat/output.dat exception, default output naming simplified to always .out of input file name. If input has out or log extension (weird), extra extension added.
  • Output file in continuous mode now looks much as it normally does in the master branch, except there's a few more headers printed and the individual cbs/findif/nbody AtomicResult outputs aren't added in real time. Running through QCFractal, the output file is expected to break down, but this hasn't been tried.
  • A Logging file has been introduced. All its contents should be considered experimental and subject to change.

Checklist

Status

  • Ready for review
  • Ready for merge
@PeterKraus
Copy link
Contributor

@PeterKraus PeterKraus commented Nov 10, 2018

This looks great. It's good to have the large monolithic chunks of driver code split into smaller parts, printing functions tucked away and not scattered across calculations. Counterpoise-corrected CBS is something I wanted to do for a while!

One point I'd like to make quite early in this PR: the whole pydantic validation mechanism is rather obscure. I understand that each Computer inherits from the base class, but I am not 100% sure how things fit together and launching what will get me where. The two tests are currently only of little help.

@dgasmith
Copy link
Member

@dgasmith dgasmith commented Nov 10, 2018

This is pretty early days on this PR. The short is that a Computer can be CBS/FindIf/N-Body/Single/etc (?) so that you can nest them and where a Single is the only object that calls quantum chemistry. For example, a N-Body can be made up of Single computations or CBS computations where a CBS computation will be made up for two or more Single computations. In this case, calling compute on N-Body will call compute on all of its children which if is a Single will call a quantum chemistry computation or if CBS will call 2 or more single computations (or FindIf!) and then construct that data into an extrapolated energy which will be given to N-Body.

The key here is that the Single can have three modes 1) a continuous computation on a single node 2) a sow/reap mode using files and 3) a sow/reap mode using QCFractal. There will assuredly be many more docs.

psi4/driver/driver_nbody.py Outdated Show resolved Hide resolved
@loriab loriab force-pushed the recursive branch 2 times, most recently from 16e1628 to 4e10c56 Mar 17, 2019
@loriab loriab changed the title Refactor Recursive Driver Refactor Recursive Driver Into Distributed Driver Mar 17, 2019
@jeffschriber
Copy link
Contributor

@jeffschriber jeffschriber commented May 24, 2019

I have some small changes to make the multilevel driver work with QCFractal. Could I get push permission, or should I open a PR to the branch?

@dgasmith
Copy link
Member

@dgasmith dgasmith commented May 24, 2019

Invite in your mailbox.

@loriab
Copy link
Member

@loriab loriab commented May 24, 2019

hooray, lgtm PR analysis is working! this may be a landslide.

@lgtm-com
Copy link

@lgtm-com lgtm-com bot commented May 24, 2019

This pull request introduces 23 alerts and fixes 14 when merging 5c73c74 into 2edf6b7 - view on LGTM.com

new alerts:

  • 14 for Unused import
  • 7 for Unused local variable
  • 1 for Except block handles 'BaseException'
  • 1 for Syntax error

fixed alerts:

  • 5 for Unused local variable
  • 5 for Unused import
  • 3 for 'import *' may pollute namespace
  • 1 for Except block handles 'BaseException'

Comment posted by LGTM.com

@lgtm-com
Copy link

@lgtm-com lgtm-com bot commented Jun 25, 2019

This pull request introduces 23 alerts and fixes 14 when merging e11bb21 into d4681c9 - view on LGTM.com

new alerts:

  • 14 for Unused import
  • 7 for Unused local variable
  • 1 for Except block handles 'BaseException'
  • 1 for Syntax error

fixed alerts:

  • 5 for Unused local variable
  • 5 for Unused import
  • 3 for 'import *' may pollute namespace
  • 1 for Except block handles 'BaseException'

@codecov-io
Copy link

@codecov-io codecov-io commented Jun 26, 2019

Codecov Report

Merging #1351 into master will decrease coverage by 0.25%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1351      +/-   ##
==========================================
- Coverage   71.15%   70.90%   -0.26%     
==========================================
  Files        1470     1468       -2     
  Lines      316070   316368     +298     
==========================================
- Hits       224906   224327     -579     
- Misses      91164    92041     +877     
Impacted Files Coverage Δ
psi4/src/psi4/libpsio/tocscan.cc 50.00% <0.00%> (-42.86%) ⬇️
psi4/src/psi4/libmints/osrecur.cc 63.25% <0.00%> (-21.56%) ⬇️
psi4/src/psi4/libfock/v.cc 83.80% <0.00%> (-10.63%) ⬇️
psi4/src/psi4/libfock/sap.cc 80.00% <0.00%> (-9.29%) ⬇️
psi4/src/psi4/scfgrad/scf_grad.cc 95.09% <0.00%> (-1.19%) ⬇️
psi4/src/psi4/liboptions/liboptions.cc 56.26% <0.00%> (-0.91%) ⬇️
psi4/src/psi4/libmints/vector.cc 62.27% <0.00%> (-0.60%) ⬇️
psi4/src/psi4/libfock/points.cc 72.24% <0.00%> (-0.27%) ⬇️
psi4/src/core.cc 82.19% <0.00%> (-0.25%) ⬇️
psi4/driver/driver.py 79.10% <0.00%> (-0.22%) ⬇️
... and 106 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7d35d14...6e36f6b. Read the comment docs.

@lgtm-com
Copy link

@lgtm-com lgtm-com bot commented Jul 18, 2019

This pull request introduces 24 alerts and fixes 14 when merging 8c0afe6 into 6e67529 - view on LGTM.com

new alerts:

  • 14 for Unused import
  • 8 for Unused local variable
  • 1 for Except block handles 'BaseException'
  • 1 for Syntax error

fixed alerts:

  • 5 for Unused local variable
  • 5 for Unused import
  • 3 for 'import *' may pollute namespace
  • 1 for Except block handles 'BaseException'

@jeffschriber jeffschriber force-pushed the recursive branch 2 times, most recently from 19f7c57 to d31ce07 Aug 12, 2019
@lgtm-com
Copy link

@lgtm-com lgtm-com bot commented Aug 13, 2019

This pull request introduces 24 alerts and fixes 14 when merging d31ce07 into b789aa3 - view on LGTM.com

new alerts:

  • 14 for Unused import
  • 8 for Unused local variable
  • 1 for Except block handles 'BaseException'
  • 1 for Syntax error

fixed alerts:

  • 5 for Unused local variable
  • 5 for Unused import
  • 3 for 'import *' may pollute namespace
  • 1 for Except block handles 'BaseException'

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

7 participants