Implement harness for DFT-D3 Python API #343
Conversation
This comment has been minimized.
This comment has been minimized.
awvwgk
force-pushed
the
dftd3
branch
6 times, most recently
from
e3c66b6
to
c7e10fe
Compare
|
At first read-through, this lgtm. Thanks very much for working on this! I am hopefully finishing up libint2 work soon. Then shifting psi4 to the new d3 and gcp implementations is next on my agenda. |
|
Friendly bump, what is necessary to move this patch forward? |
|
lgtm, thank you! Fwiw, I don't find any interference wrt the older implementations in downstream testing so far. Would you like to rebase so correct CI lanes run, or would you like me to rebase and force-push to your branch?
Apologies, the Psi4 release got busy. I held out hope until the beginning of May that we could transition to the new harnesses, but I didn't make it. |
- fetch D3 damping parameters from dftd3.parameters if available - adjust CI to allow testing of DFT-D3 Python API
|
btw, if you do the rebase, be aware I added this to get tests to pass https://github.com/MolSSI/QCEngine/blob/master/devtools/conda-envs/opt-disp.yaml#L14 . Possibly it interferes with this PR. |
|
Will have to check, I had the idea to update the CODATA version in all our programs, however the test accuracy is lower than the change in conversion factors... |
|
From what I remember differences were quite small. Maybe physconst change effect on the geometry -- I don't remember if it originates in angstroms. |
|
Small enough to trip off the contributed tests unfortunately, I'll have to check the reference values for DFT-D3 and DFT-D4 to make sure the CODATA change doesn't affect any energy / gradient in the last digits. |
Any suggestions on solving the test failure? Do reference results or physconst need adjusting here? |
I need to update the reference results, which is quite unfortunate. Has been a while since I looked last into this patch, currently have a couple of other projects I want to move forward now that my only have three months of my PhD time in Bonn left. Hopefully I'll come back to this one soon. |
Understood -- finishing up definitely takes priority. If it's a matter of reference values, not correctness, would it be reasonable if I loosened the checks and filed an issue to be revisited later? Or was it contributions from physconst vs. correctness that you wanted to check up on first? |
|
Loosening the thresholds is an option. |
| @@ -662,6 +662,21 @@ | |||
| "hf": {"params": {"s6": 1.000, "s8": 0.713190, "a1": 0.079541, "a2": 3.627854}}, # JBS 01/2021 | |||
| }, | |||
| }, | |||
| "d3op": { | |||
| "formal": "D3(op)", | |||
There was a problem hiding this comment.
| "formal": "D3(op)", | |
| "formal": "D3(OP)", |
can this be all-caps for consistency? I suspect psi4 is expecting it so.
There was a problem hiding this comment.
It was called D3(op) with lowercase op in the original publication: https://pubs.acs.org/doi/10.1021/acs.jctc.7b00176.
There was a problem hiding this comment.
I think D3(bj) was published with mixed-case, too. Maybe that's the better direction. Deferred for now.
|
|
||
| However, we are not happy with the structure of the dict and | ||
| therefore do some restructuring. The below solution tries to bend | ||
| the data returned by dftd3 into the right shape to make qcng happy. |
There was a problem hiding this comment.
thanks. and sorry for the historical necessity.
There was a problem hiding this comment.
At least I learned something about the things one can do with dict-comprehension in Python.
Closes #341
Description
Implement harness for DFT-D3 Python API with native QCSchema support. Allows access to the parameter database and adds those entries to the definitions in the dispersion resources.
Changelog description
Implement harness for DFT-D3 Python API
Status