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

empirical disp updates #2142

Merged
merged 6 commits into from
Sep 27, 2021
Merged

empirical disp updates #2142

merged 6 commits into from
Sep 27, 2021

Conversation

loriab
Copy link
Member

@loriab loriab commented Mar 26, 2021

Description

Todos

  • switch gcp interface to QCEngine
  • add dftd4 interface calling QCEngine
  • update types return for disp. engine can handle ndarray, so leave arrays as np and shaped rather than flat lists
  • more tests -- psiapi, parameters extend func
  • add docs
  • note min qcng version, prob. v0.19
  • this passes cleanly locally but that's with custom engine and dftd4, so several PRs ahead of this one
  • allow doi as citations for dft
  • note that for Mol.run_dftd4, func overrides parameters in keeping with dftd4 API behavior, whereas in dftd3, parameters extend or override func.

Questions

  • @jeffschriber should fisapt grab 2-body disp analysis or total disp analysis
  • a couple of the interface updates should be in v1.4 but d4 itself can be in v1.5. so this can get split after upstream settles down.

Checklist

Aug 2021 Notes

  • rebased. the only thing that's left of the original PR checklist is dftd4 itself. gcp and dftd3 updates were pulled in in empirical disp updates for qcng v0.19 #2180.
  • note that it's not the dftd4 exe that this (that is, qcng) needs; rather, it's the dftd4 pymod that's needed.
  • one can use the c-f 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 a dftd4 conda package (not in final build form) off -c psi4/label/dev.

Status

  • Ready for review
  • Ready for merge

EDIT: closes #1710

@lgtm-com
Copy link

lgtm-com bot commented Mar 26, 2021

This pull request fixes 5 alerts when merging 79d4582 into 59d998d - view on LGTM.com

fixed alerts:

  • 2 for 'import *' may pollute namespace
  • 1 for Unused local variable
  • 1 for Module-level cyclic import
  • 1 for Module is imported with 'import' and 'import from'

@lgtm-com
Copy link

lgtm-com bot commented Mar 27, 2021

This pull request fixes 5 alerts when merging 1e663e4 into 86700a9 - view on LGTM.com

fixed alerts:

  • 2 for 'import *' may pollute namespace
  • 1 for Unused local variable
  • 1 for Module-level cyclic import
  • 1 for Module is imported with 'import' and 'import from'

@lgtm-com
Copy link

lgtm-com bot commented May 6, 2021

This pull request fixes 5 alerts when merging 051af5c into 966d1bd - view on LGTM.com

fixed alerts:

  • 2 for 'import *' may pollute namespace
  • 1 for Unused local variable
  • 1 for Module-level cyclic import
  • 1 for Module is imported with 'import' and 'import from'

@lgtm-com
Copy link

lgtm-com bot commented May 8, 2021

This pull request fixes 5 alerts when merging f1483ae into b4a272f - view on LGTM.com

fixed alerts:

  • 2 for 'import *' may pollute namespace
  • 1 for Unused local variable
  • 1 for Module-level cyclic import
  • 1 for Module is imported with 'import' and 'import from'

@lgtm-com
Copy link

lgtm-com bot commented May 12, 2021

This pull request fixes 5 alerts when merging 97583a2 into d9d8477 - view on LGTM.com

fixed alerts:

  • 2 for 'import *' may pollute namespace
  • 1 for Unused local variable
  • 1 for Module-level cyclic import
  • 1 for Module is imported with 'import' and 'import from'

@loriab loriab added external-interface For issues about interfaces with external programs: ADCC, CheMPS2, GDMA, MRCC... feature Extends an existing Psi feature or develops a new one. labels Aug 7, 2021
Copy link
Contributor

@JonathonMisiewicz JonathonMisiewicz left a 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.

doc/sphinxman/source/dftd3.rst Outdated Show resolved Hide resolved
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.
Copy link
Contributor

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
Copy link
Contributor

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?

Copy link
Member Author

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

@JonathonMisiewicz
Copy link
Contributor

Also, please grep for comments related to dftd3 in the code and update them as needed. proc.py::scf_helper certainly needs to be updated.

Copy link
Contributor

@PeterKraus PeterKraus left a 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.

doc/sphinxman/source/dftd3.rst Outdated Show resolved Hide resolved
-----
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`.
Copy link
Contributor

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?

Copy link

@awvwgk awvwgk Aug 20, 2021

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.

"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`.
Copy link
Contributor

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.

Copy link
Member Author

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.

Copy link
Contributor

@PeterKraus PeterKraus left a 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!

@PeterKraus PeterKraus merged commit 2d33c3f into psi4:master Sep 27, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
external-interface For issues about interfaces with external programs: ADCC, CheMPS2, GDMA, MRCC... feature Extends an existing Psi feature or develops a new one.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

DFTD4 interface in Psi4
5 participants