MolSSI / QCEngine

Quantum chemistry program executor and IO standardizer (QCSchema).
https://molssi.github.io/QCEngine/
BSD 3-Clause "New" or "Revised" License
163 stars 79 forks source link

Harness for DFT-D3 Python API #341

Closed awvwgk closed 2 years ago

awvwgk commented 2 years ago

Is your feature request related to a problem? Please describe.

The s-dftd3 project provides a reimplementation of the DFT-D3 method with native bindings to Python (API docs) since version 0.4.0, the bindings are similar to the Python API provided by dftd4, which already has a harness in qcng.

Therefore, the implementation of a hardness should boil down to change the import from dftd4 to dftd3 and adapting the dashlevel logic.

Describe the solution you'd like

Possible options are:

Describe alternatives you've considered

Do nothing, there is already an interface to the reference dftd3 implementation.

Additional context

loriab commented 2 years ago

I haven't thought through the possible stumbling blocks, but in general, I'd suggest deriving harnesses where we can (like classic vs. mctc gcp). Does it make sense for s-dftd3's harness to derive from dftd4? SDFTD3Harness sounds good. The harness to psi4-fork-of-dftd3 will need to be maintained at least until May (when psi4 v1.6 appears), and I wouldn't mind keeping it around permanently read-only if it isn't a burden. QCEngine's mission is to wrap QC programs' decisions, so following the params or fctl logic of dftd4 rather than the current layering of dftd3 is probably right. Thanks for working on this!

awvwgk commented 2 years ago

I haven't thought through the possible stumbling blocks

There are some, it boils down to the question whether we want a bug-compatible implementation or whether it is sufficient to get the same dispersion energies and gradients in the end. My greatest concerns are

Notably, there is a bug in DFTD3Harness at

https://github.com/MolSSI/QCEngine/blob/bc1a161d301eeffcdf6a26103b754d256db41ce9/qcengine/programs/dftd3.py#L313

which removes alpha6 from the rational damping parameters, while it actually an essential part of the three-body correction for all damping functions. However, the definition of a separate atmgr dispersion correction and the double evaluation of D3 hides the bug.

loriab commented 2 years ago

Oh dear. No need in principle to perpetuate my mistakes. I'd like to understand the trouble better. Since the original d3-zero didn't include a 3-body contribution, we continued that -d3* to mean 2-body only. Is this the discrepancy between psi-flavored-dftd3 and s-dftd3? That is, does the official definition of -d3bj include 3-body?

awvwgk commented 2 years ago

No worries, the resulting dispersion correction is correct, only the implementation has a bug that is not realized in any actual calculation due to performing the D3 calculation twice.

There is actually a difference in naming between the two-body only variants, D3(*), and the ones that include three-body contributions, D3(*)-ATM. However, there was never a clear recommendation, which variant should be preferred with the original publications, but in practice we have been using mostly the ATM variants (AFAIK from the works I have seen since 2017). Basically this is our mess, which I would like to handle more gracefully going forward. We learned from this mistake and made a clear recommendation in the D4 publication.

Another thing is that no damping function really defined a different ATM damping function, the D3M(0) would have been the best candidate to introduce the β parameter there as well, however most revised damping functions, like D3M, D3(op) and D3(CSO), didn't really discuss how the ATM should be handled. Nowadays we know that also rational damping functions work well with the ATM term.

awvwgk commented 2 years ago

I created an initial draft for the DFT-D3 Python API harness in #343. Feedback is welcome.