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

Implement harness for DFT-D3 Python API #343

Closed awvwgk closed 2 years ago

awvwgk commented 2 years ago

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

codecov[bot] commented 2 years ago

Codecov Report

Merging #343 (63d12dc) into master (113d1e8) will increase coverage by 0.52%. The diff coverage is 94.17%.

awvwgk commented 2 years ago

Any idea why conda is solving for dftd3-python 0.4.1 rather than the latest? Mamba does find the correct version just fine.

``` ❯ mamba env create -n qcng-test --file devtools/conda-envs/xtb.yaml ... ❯ mamba activate qcng-test ❯ mamba list # packages in environment at /home/awvwgk/software/opt/conda/envs/qcng-test: # # Name Version Build Channel _libgcc_mutex 0.1 conda_forge conda-forge _openmp_mutex 4.5 1_gnu conda-forge attrs 21.4.0 pyhd8ed1ab_0 conda-forge brotlipy 0.7.0 py310h6acc77f_1003 conda-forge bzip2 1.0.8 h7f98852_4 conda-forge ca-certificates 2021.10.8 ha878542_0 conda-forge certifi 2021.10.8 py310hff52083_1 conda-forge cffi 1.15.0 py310h0fdd8cc_0 conda-forge charset-normalizer 2.0.10 pyhd8ed1ab_0 conda-forge codecov 2.1.11 pyhd3deb0d_0 conda-forge coverage 6.2 py310h6acc77f_0 conda-forge cryptography 36.0.1 py310h685ca39_0 conda-forge dftd3-python 0.5.1 py310h2b7ec95_0 conda-forge dftd4 3.3.0 hf06ca72_3 conda-forge dftd4-python 3.3.0 py310hf56f200_1 conda-forge gcp-correction 2.3.1 h1990efc_3 conda-forge idna 3.3 pyhd8ed1ab_0 conda-forge importlib-metadata 4.10.1 py310hff52083_0 conda-forge importlib_metadata 4.10.1 hd8ed1ab_0 conda-forge iniconfig 1.1.1 pyh9f0ad1d_0 conda-forge ld_impl_linux-64 2.36.1 hea4e1c9_2 conda-forge libblas 3.9.0 13_linux64_openblas conda-forge libcblas 3.9.0 13_linux64_openblas conda-forge libffi 3.4.2 h7f98852_5 conda-forge libgcc-ng 11.2.0 h1d223b6_11 conda-forge libgfortran-ng 11.2.0 h69a702a_11 conda-forge libgfortran5 11.2.0 h5c6108e_11 conda-forge libgomp 11.2.0 h1d223b6_11 conda-forge liblapack 3.9.0 13_linux64_openblas conda-forge libnsl 2.0.0 h7f98852_0 conda-forge libopenblas 0.3.18 pthreads_h8fe5266_0 conda-forge libstdcxx-ng 11.2.0 he4da1e4_11 conda-forge libuuid 2.32.1 h7f98852_1000 conda-forge libzlib 1.2.11 h36c2ea0_1013 conda-forge mctc-lib 0.2.4 h92ddd45_2 conda-forge ncurses 6.2 h58526e2_4 conda-forge numpy 1.22.1 py310h454958d_0 conda-forge openssl 1.1.1l h7f98852_0 conda-forge packaging 21.3 pyhd8ed1ab_0 conda-forge pint 0.18 pyhd8ed1ab_0 conda-forge pip 21.3.1 pyhd8ed1ab_0 conda-forge pluggy 1.0.0 py310hff52083_2 conda-forge psutil 5.9.0 py310h6acc77f_0 conda-forge py 1.11.0 pyh6c4a22f_0 conda-forge py-cpuinfo 8.0.0 pyhd8ed1ab_0 conda-forge pycparser 2.21 pyhd8ed1ab_0 conda-forge pydantic 1.9.0 py310h6acc77f_0 conda-forge pyopenssl 21.0.0 pyhd8ed1ab_0 conda-forge pyparsing 3.0.7 pyhd8ed1ab_0 conda-forge pysocks 1.7.1 py310hff52083_4 conda-forge pytest 6.2.5 py310hff52083_2 conda-forge pytest-cov 3.0.0 pyhd8ed1ab_0 conda-forge python 3.10.2 h62f1059_0_cpython conda-forge python_abi 3.10 2_cp310 conda-forge pyyaml 6.0 py310h6acc77f_3 conda-forge qcelemental 0.24.0 pyhd8ed1ab_0 conda-forge readline 8.1 h46c0cb4_0 conda-forge requests 2.27.1 pyhd8ed1ab_0 conda-forge setuptools 60.5.0 py310hff52083_0 conda-forge simple-dftd3 0.5.1 hb01ceae_0 conda-forge six 1.16.0 pyh6c4a22f_0 conda-forge sqlite 3.37.0 h9cd32fc_0 conda-forge tk 8.6.11 h27826a3_1 conda-forge toml 0.10.2 pyhd8ed1ab_0 conda-forge toml-f 0.2.2 h92ddd45_2 conda-forge tomli 2.0.0 pyhd8ed1ab_1 conda-forge typing-extensions 4.0.1 hd8ed1ab_0 conda-forge typing_extensions 4.0.1 pyha770c72_0 conda-forge tzdata 2021e he74cb21_0 conda-forge urllib3 1.26.8 pyhd8ed1ab_1 conda-forge wheel 0.37.1 pyhd8ed1ab_0 conda-forge xtb 6.4.1 hf06ca72_2 conda-forge xtb-python 20.2 py310h6acc77f_4 conda-forge xz 5.2.5 h516909a_1 conda-forge yaml 0.2.5 h7f98852_2 conda-forge zipp 3.7.0 pyhd8ed1ab_0 conda-forge zlib 1.2.11 h36c2ea0_1013 conda-forge ```

Probably just have to set an explicit version requirement.

Edit: Looks like the opt-dispersion.yaml environment does not solve with mamba under the constraints in the CI:

❯ mamba env create -n qcng-test --file test.yaml
psi4/label/dev/noarch    [====================] (00m:00s) Done
psi4/label/dev/linux-64  [====================] (00m:00s) Done
conda-forge/noarch       [====================] (00m:02s) Done
conda-forge/linux-64     [====================] (00m:06s) Done

Looking for: ['psi4', 'blas=[build=mkl]', 'intel-openmp!=2019.5', 'rdkit', 'dftd3==3.2.1', "dftd3-python[version='>=0.5.1']", 'dftd4-python', "mp2d[version='>=1.1']", 'gcp', 'geometric', 'optking', 'pymdi', 'python', 'pyyaml', 'py-cpuinfo', 'psutil', "qcelemental[version='>=0.24.0']", "pydantic[version='>=1.0.0']", 'msgpack-python', 'pytest', 'pytest-cov', 'codecov', 'pip', 'python=3.8']

Encountered problems while solving:
  - nothing provides requested intel-openmp !=2019.5
  - nothing provides gcc-5-mp needed by psi4-1.2a1.dev419+809f363-py36_0
❯ cat test.yaml
name: qcng-test
channels:
  - psi4/label/dev
  - conda-forge
dependencies:
  - psi4
  - blas=*=mkl
  - intel-openmp!=2019.5
  - rdkit
  - dftd3 3.2.1
  - dftd3-python >=0.5.1
  - dftd4-python
  - mp2d >=1.1
  - gcp
  - geometric
  - optking
  - pymdi
  - python
  - pyyaml
  - py-cpuinfo
  - psutil
  - qcelemental >=0.24.0
  - pydantic>=1.0.0
  - msgpack-python
  - pytest
  - pytest-cov
  - codecov
  - pip
  - pip:
      - pyberny
  - python=3.8
loriab commented 2 years ago

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.

awvwgk commented 2 years ago

Friendly bump, what is necessary to move this patch forward?

loriab commented 2 years ago

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?

Friendly bump, what is necessary to move this patch forward?

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.

loriab commented 2 years ago

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.

awvwgk commented 2 years ago

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...

loriab commented 2 years ago

From what I remember differences were quite small. Maybe physconst change effect on the geometry -- I don't remember if it originates in angstroms.

awvwgk commented 2 years ago

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.

loriab commented 2 years ago

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?

awvwgk commented 2 years ago

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.

loriab commented 2 years ago

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?

awvwgk commented 2 years ago

Loosening the thresholds is an option.