search

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

bugfix: 'N Atoms' is now set in qcvars for grad/Hessian calcs #323

Closed eljost closed 2 years ago

eljost commented 2 years ago

Description

The update to 0.20.0 broke the Turbomole harness, as 'N ATOMS' was missing in qcvars, when derivatives of the energy were calculated. This line led to an exception:

https://github.com/MolSSI/QCElemental/blob/36c5e0d3de3655fe02c42c4c1fe52eb4ba933c54/qcelemental/models/results.py#L279

Changelog description

qcvars['N ATOMS'] is now set for gradient and/or Hessian runs, so the AtomicProperties can be now be properly created.

Status

codecov[bot] commented 2 years ago

Codecov Report

Merging #323 (0ffc5a3) into master (78bd7d2) will decrease coverage by 0.05%. The diff coverage is 37.50%.

sheepforce commented 2 years ago

Shall I mint a v0.20.1 to help downstream?

I would very much appreciate a bug fix release; it is easier to package a small release than backporting the changes (https://github.com/NixOS/nixpkgs/blob/master/pkgs/development/python-modules/qcengine/default.nix)

eljost commented 2 years ago

Why does the test_harness_canonical need to be commented?

Because, as far as I can tell, the qcsk_bs basis is not a simple, common basis set name as 6-31G, but a QCSchema basis set?! This is not handled by the TurbomoleHarness, so the test fails. That is why I disabled it for now.

loriab commented 2 years ago

Why does the test_harness_canonical need to be commented?

Because, as far as I can tell, the qcsk_bs basis is not a simple, common basis set name as 6-31G, but a QCSchema basis set?! This is not handled by the TurbomoleHarness, so the test fails. That is why I disabled it for now.

You're right about that being a qcschema basisset. No qcengine harness can handle such basis sets right now, even though the schema allow them, so that test just tests that a sensible error message is thrown, rather than harnesses reacting in a dozen different ways. Perhaps add something like https://github.com/MolSSI/QCEngine/blob/master/qcengine/programs/nwchem/runner.py#L178-L179 to the turbomole harness to allow it to pass.

eljost commented 2 years ago

Ah, ok. I'll come up with something tomorrow.

eljost commented 2 years ago

I re-enabled the test and added handling for QCSchema basis set input. I basically just copy&pasted the lines from your link. The previously disabled test passes now.

loriab commented 2 years ago

Thanks again for the fix, @eljost. The 0.20.1 release including this is up on pypi and will eventually percolate to conda, @sheepforce.