-
Notifications
You must be signed in to change notification settings - Fork 455
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
empirical disp updates #2142
empirical disp updates #2142
Conversation
This pull request fixes 5 alerts when merging 79d4582 into 59d998d - view on LGTM.com fixed alerts:
|
This pull request fixes 5 alerts when merging 1e663e4 into 86700a9 - view on LGTM.com fixed alerts:
|
This pull request fixes 5 alerts when merging 051af5c into 966d1bd - view on LGTM.com fixed alerts:
|
This pull request fixes 5 alerts when merging f1483ae into b4a272f - view on LGTM.com fixed alerts:
|
This pull request fixes 5 alerts when merging 97583a2 into d9d8477 - view on LGTM.com fixed alerts:
|
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.
A few documentation comments, but LGTM otherwise.
psi4/driver/qcdb/molecule.py
Outdated
dashlvl : str, optional | ||
Name of dispersion correction to be applied (e.g., d, D2, | ||
d3(bj), das2010). Must be key in `dashcoeff` or "alias" or | ||
"formal" to one. |
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.
Do you mean to run?
Parameters | ||
---------- | ||
func : str, optional | ||
Name of functional (func only, func & disp, or disp only) for |
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.
What if this isn't supplied?
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.
mol.run_dftd4(dashlvl="d4bj")
throws a none is not an allowed value (type=type_error.none.not_allowed)
The validation for d4 isn't quite so thorough as d3 because the d4 program uses different precedence rules, so I couldn't reuse by d3 fn. But the testing isn't quite as stingy as it looks here at psi4: https://github.com/MolSSI/QCEngine/blob/master/qcengine/programs/tests/test_dftd3_mp2d.py#L1701-L1715
Also, please |
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.
This might be a good place to add at least the most common -D4 functionals to test_dft_benchmark.py
.
----- | ||
This function wraps the QCEngine dftd4 harness which wraps the internal DFTD4 Python API. | ||
As such, the upstream convention of `func` trumping `dashparam` holds, rather than the | ||
:py:func:`run_dftd3` behavior of `dashparam` extending or overriding `func`. |
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.
Is there any way to harmonise these two? The way I understand it, we do the same thing Psi4-side, but the behaviour of dftd3 and dftd4 differs when both func and dashparams are provided.
In any case, not Psi4's but rather qcng's problem...
I'm guessing the only problem might be when one wants to run, say, B3LYP (for which func
values are defined) with a custom set of parameters - is this possible to do with dftd4?
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.
See related discussion here: dftd4/dftd4#98
Short summary:
dftd4
does not provide the possibility to select a functional, say B3LYP and only change the s8, this is by design. Either the downstream user hands the responsibility over to dftd4
to select the correct parameters or provides all damping parameters.
If Psi4 requires full control over the parameter selection, it can't use the former mechanism, but has to always construct the set of damping parameters first (qcng
knows how to do this for all supported functionals in dftd4
).
Note that in the future dftd4
will most likely throw an error in case both functional name and an (incomplete) set of damping parameters are provided.
psi4/driver/qcdb/molecule.py
Outdated
"formal" to run. | ||
dashparam : dict, optional | ||
Values for the same keys as `dashcoeff[dashlvl]['default']` | ||
used to override any or all values initialized by `func`. |
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.
Given your comment below, this is not true for dftd4.
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 revised the docstring, I hope truthfully. There were several principles that wanted to be followed here ((a) psi4 dft-d interface should be consistent, (b) qcengine harness should follow upstream project in keyword handling, (c) qcengine dash harnesses (dftd3, gcp, mp2d, dftd4) should be consistent, (d) Molecule.run_dftdN (aka psi4 -d interface) should be consistent), and I just couldn't make them all so. The Molecule.run_dftdN
seemed like the least important one to sacrifice.
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.
Looks great, can't wait to use the functionals!
Description
Todos
Mol.run_dftd4
, func overrides parameters in keeping with dftd4 API behavior, whereas in dftd3, parameters extend or override func.Questions
Checklist
Aug 2021 Notes
dftd4-python
conda package if you know how to set up your env to install both it and psi4 deps. But for linux only, I've prepared adftd4
conda package (not in final build form) off-c psi4/label/dev
.Status
EDIT: closes #1710