Implement support for DFT-D4

[awvwgk]

Description

• implement harness for dftd4 Python API (closes Implement DFT-D4 harness #174)
• Finish upstream implementation (see Enable native support for QCSchema input dftd4/dftd4#70)
• Publish Python API on conda-forge (see Add recipe for dftd4-python conda-forge/staged-recipes#14084)
• Add a few regression tests
• Add DFT-D4 to tests in this repository (requires cf package, upstream implementation and new dftd4 release)

Note:
The Python API must be explicitly enabled when building dftd4 or can exist separately from a dftd4 installation.
The harness can therefore fail to use dftd4 even if a dftd4 installation is present, in this case it could have a fallback for a FIFO calculator (xyz + command line input, JSON dump output), but this is work for a different PR.

Changelog description

Implement support for DFT-D4

Status

• Code base linted
• Ready to go

[codecov]

Codecov Report

Merging #291 (59271ad) into master (7ac0f54) will increase coverage by 0.90%.
The diff coverage is 96.03%.

[loriab]

Note:
The Python API must be explicitly enabled when building dftd4 or can exist separately from a dftd4 installation.
The harness can therefore fail to use dftd4 even if a dftd4 installation is present, in this case it could have a fallback for a FIFO calculator (xyz + command line input, JSON dump output), but this is work for a different PR.

Can def found look for both which_import(dftd4) and then check separately for the API component or the external API package before declaring the program found? similar to https://github.com/MolSSI/QCEngine/blob/master/qcengine/programs/nwchem/runner.py#L55

[loriab]

don't worry about nwchem::test_error_correction -- it's ~20% flaky

[awvwgk]

Can def found look for both which_import(dftd4) and then check separately for the API component or the external API package before declaring the program found?

This should be the way to go, for now I want to just flesh out the Python API of dftd4. I had a brief look into the dftd3 FIFO harness which is incredible more complex than using a Python API, therefore this is nothing I want to tackle now.

I would be more interesting if the proposed interface is optimal for your purpose. I'm using the keywords to specify the damping parameters or the method name that should be looked up in the internal database. For dftd3 is handled a bit differently, what is there preference here?

[loriab]

I would be more interesting if the proposed interface is optimal for your purpose. I'm using the keywords to specify the damping parameters or the method name that should be looked up in the internal database. For dftd3 is handled a bit differently, what is there preference here?

The Python API alone will be great, thank you. No need for file + cmdline route -- that was just the only way circa 2012.

If it's all the same to you, would you adapt the fields toward https://github.com/psi4/psi4/blob/master/psi4/driver/procrouting/empirical_dispersion.py#L193-L209 ? Main changes are:

• d4 goes into keywords.level_hint (https://github.com/psi4/psi4/blob/master/psi4/driver/procrouting/empirical_dispersion.py#L58)
• any explicit parameters go into keywords.params_tweaks as a dictionary https://github.com/psi4/psi4/blob/master/psi4/driver/procrouting/empirical_dispersion.py#L62-L65
• model.method be, for example, pbe-d4 if unperturbed or empty string if parameters at all perturbed https://github.com/psi4/psi4/blob/master/psi4/driver/procrouting/empirical_dispersion.py#L68-L71

d3/d4 don't fit too well into the AtomicInput layout, unfortunately, but the above would be helpful in integrating into psi if it's not too much trouble.

[awvwgk]

Sounds good to me, DFT-D4 will provide this interface in the Python API for running QCSchema input. The only particularity, I don't allow to overwrite individual damping parameters, so either a method name or all parameters are required.

[loriab]

The only particularity, I don't allow to overwrite individual damping parameters, so either a method name or all parameters are required.

That's no problem for psi, as once user input passes through the resolver, a full set of parameters is always provided.

Thanks for the changes.

[awvwgk]

DFT-D4 3.1.0 is rolled out to conda-forge, just another hour until it propagates through the CDN and we can use it here to setup the testing environment.

[awvwgk]

What is the current status of this patch? It has been stale for a while now, is there anything I can do to help move this forward?

[loriab]

What is the current status of this patch? It has been stale for a while now, is there anything I can do to help move this forward?

I think it's great and good to merge. I'm sorry for the delay; I was hoping to get awvwgk#63 merged into this beforehand, but then I got a cycle behind you on dftd4 revisions and didn't revisit. Shall we get this merged sooner, and then I'll propose updates once I've rechecked the abovementioned harness PR and psi4/psi4#2142 ?

[lgtm-com]

This pull request introduces 1 alert when merging 8fdefe5 into 43639f2 - view on LGTM.com

new alerts:

• 1 for Except block handles 'BaseException'

[lgtm-com]

This pull request introduces 1 alert when merging e805d1b into 43639f2 - view on LGTM.com

new alerts:

• 1 for Except block handles 'BaseException'

[awvwgk]

@loriab, sorry, I didn't see the PR against my fork, I usually don't watch my own forks because I have bots syncing them with upstream via PRs, so 99% of the activity there is usually noise (or myself).

I had a quick look over your changes and added a few adjustments to the parameter reading. I also wonder why qcng is calling it d4bj rather than going with the actual name which is d4?

[awvwgk]

There are a few tests from test_dftd3 now failing, but I guess those are the added tests for dftd4. I think we should come up with a clearer naming here, otherwise this is just confusing.

Any idea why those fail? Did I break something upstream already?

[lgtm-com]

This pull request introduces 1 alert when merging f83605a into 43639f2 - view on LGTM.com

new alerts:

• 1 for Unused import

[awvwgk]

I might have to look into the damping parameter generation again, we might be propagating a bad API decision from the C or Python layer up into qcng here (see dftd4/dftd4#98). It might be worth to make both method and param_tweaks input an error at the Python level in dftd4, catch in the qcschema runner in dftd4 and send it back as ComputeError to qcng at some point until dftd4 is actually able to handle this case (load method first, overwrite parameters one by one).

[loriab]

Thanks for merging and all the patching. I've made a couple suggestions, but with those, the psi4 PR seems to be working, too. Will rebase psi4 and do another check tomorrow.

[loriab]

Suggested change

	"default": collections.OrderedDict([("s8", 1.0), ("a1", 1.0), ("a2", 1.0), ("s9", 1.0)]),
	"default": collections.OrderedDict([("s8", 1.0), ("a1", 1.0), ("a2", 1.0), ("s9", 1.0)]),
	"default": collections.OrderedDict(
	[("s8", 1.0), ("a1", 1.0), ("a2", 1.0), ("s9", 1.0), ("s6", 1.0), ("alp", 16.0)]
	),

[loriab]

Was renaming the reference values like 89f4676#diff-c24ed775befa47b0b9de0da35810c7a47d5a6c5aa7c35b3e892a8b12abb09de6L78 solely to get the tests to work? Or do you very much not want the current definition of D4 to appear under the D4(BJ) label? I ask because I think that renaming of the refs is hiding another problem. Right now without damping popped, everything is getting caught in https://github.com/MolSSI/QCEngine/pull/291/files#diff-db09fb462ae1f6675bd709a50e8180a58d94038aedc9331b2f71d47e8b0c9d93R90 . Generally, I try to make the extras["qcvars"] as specific as possible, so no matter if model.method is b3lyp-d4 or b3lyp-d4(bj), the qcvar is the latter.

I can fix all this up. But I'd like to (1) not have damping in the params list https://github.com/MolSSI/QCEngine/pull/291/files#diff-d3e3a63b874003f6347c470b383ecfc60206b7c05b53f03a03f9e0ca04f081b3R806 since it's already specified at L798 and (2) have the qcvar label be D4(BJ) for reliability rather than whatever's in model.method. Is either change unsavory to you? Alternately on (2), we can have both D4(BJ) and D4 stored. (But only for D4=D4(BJ) among aliases, as I don't want to declare D=D2 https://github.com/MolSSI/QCEngine/blob/master/qcengine/programs/empirical_dispersion_resources.py#L19) :-)

[awvwgk]

Naming the dashlevel D4(BJ) is not completely accurate actually, we defined D4 as alias for D4(BJ, EEQ)-ATM in the original DFT-D4 publication, which was one of four potential methods we tested in the course of that work (D4(BJ, GFN2-xTB)-ATM, D4(BJ, GFN2-xTB)-MBD and D4(BJ, EEQ)-MBD were the other three). Therefore, I'm not completely happy with D4(BJ) because it misses the actual important information on the dispersion model.

[loriab]

Naming the dashlevel D4(BJ) is not completely accurate actually, we defined D4 as alias for D4(BJ, EEQ)-ATM in the original DFT-D4 publication, which was one of four potential methods we tested in the course of that work (D4(BJ, GFN2-xTB)-ATM, D4(BJ, GFN2-xTB)-MBD and D4(BJ, EEQ)-MBD were the other three). Therefore, I'm not completely happy with D4(BJ) because it misses the actual important information on the dispersion model.

Makes sense. Not recording the 3-body aspect was worrying me a bit, too. See what you think of awvwgk#65, please. It gives me a clean test suite for qcng and for psi4 psi4/psi4#2142.

Also feel free to rebase to incorporate #297 test suite fixes.

[loriab]

Thanks for the merge. I've push a commit directly to try to fix the d4 CI.

[loriab]

lgtm! Thanks for all the extra features and your patience with changes for psi4.

Ready to merge in your view?

[loriab]

FYI, upon import psi4, psi4 processes the QCEngine empirical_dispersion_parameters file and thus now the parameters file from dftd4-python if present. That can crash if this line allowing DOIs isn't present, as it is now in psi4 master. I mention this in case someone has an older psi4 in the presence of dftd4-python and raises the issue with you instead of me, the guilty party.

[awvwgk]

Can we make the setup of the parameters for DFT-D4 lazy? Like just populate the fields if somebody actually asks for them when calling DFTD4 or constructing damping parameters? This would greatly reduce the probability that somebody accidentally runs into this issue. (my bad, that wasn't actually the problem)

Maybe this will not trip off the check in Psi4? I have to admit it's a bit hacky.

diff --git a/qcengine/programs/empirical_dispersion_resources.py b/qcengine/programs/empirical_dispersion_resources.py
index f89bfd7..adbe4f1 100644
--- a/qcengine/programs/empirical_dispersion_resources.py
+++ b/qcengine/programs/empirical_dispersion_resources.py
@@ -866,7 +866,7 @@ def _get_d4bj_definitions() -> dict:
             citation = params.pop("doi", None)
             definitions[method] = dict(
                 params=params,
-                citation=citation,
+                citation="    " + citation + "\n",
             )
         except KeyError:
             continue

[loriab]

Maybe this will not trip off the check in Psi4? I have to admit it's a bit hacky.

Yes, that works nicely, thanks.

[awvwgk]

Thanks a lot for your help. It took a long way to finally make DFT-D4 available here, but I think the dftd4 project has profited a lot from this collaboration.