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

Hessian parsing fixes for Turbomole Harness #349

Closed eljost closed 2 years ago

eljost commented 2 years ago

Description

Parsing Turbomole Hessian was faulty for bigger molecules. This was found here. As the present code in QCEngine is derived from my own project pysisyphus I want to provide my fix here.

I didn't add test for a big Hessian, but I have a 500 kb Hessian file at hand. If you tell me where I could put this file I could include a proper test. I refrained from including the file inline as a pytest.fixture ...

Changelog description

Fixed Hessian-parsing for bigger molecules >= 60 atoms.

Status

Regarding the linting ... i formatted the files I modified using Black, but there appear to be other "unlinted" files, which I did not modify here: 

make format
[...]
black qcengine
reformatted qcengine/config.py
reformatted qcengine/programs/cfour/runner.py
reformatted qcengine/programs/gamess/runner.py
reformatted qcengine/programs/nwchem/runner.py
reformatted qcengine/programs/molpro.py

All done! ✨ 🍰 ✨
5 files reformatted, 96 files left unchanged.

Locally, I ran all Turbomole tests and they passed.

codecov[bot] commented 2 years ago

Codecov Report

Merging #349 (f50ff4f) into master (dd5eec6) will increase coverage by 0.30%. The diff coverage is 84.00%.

lgtm-com[bot] commented 2 years ago

This pull request introduces 1 alert when merging 646f6c9baf60d951812e40a386ddb3dcb6221046 into aeead6fa1327917c7b9fd874479e013114b45b3e - view on LGTM.com

new alerts:

loriab commented 2 years ago

I didn't add test for a big Hessian, but I have a 500 kb Hessian file at hand. If you tell me where I could put this file I could include a proper test. I refrained from including the file inline as a pytest.fixture ...

I apologize for the delay -- I didn't know how to answer this. A relevant statistic is that qcng presently installed is 1.8 MB. Alternatives going forward:

  1. add the big file to tests and use it in a pytest and nevermind the extra bulk
  2. follow the pattern in qcel
  3. don't bother with the big hessian test or defer it to a future PR

If you want to make changes to the PR, please rebase first so that the new CI lanes run. Or, if you're content with the current contents and don't have the branch handy, lmk, and I can rebase and force-push the branch.

Thank you again.

loriab commented 2 years ago

Ok, #369 confirms that this PR is RTG after a rebase. If I don't hear concerns before Friday, I'll force-push the rebased branch to here and merge as-is. Thanks!

eljost commented 2 years ago

I rebased onto the current master. And regarding the Hessian I think I'll stick with option 3. for now ;)