Skip to content

Revert breaking API changes on master branch #1498

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

Merged
merged 3 commits into from
Oct 30, 2024

Conversation

berquist
Copy link
Member

@berquist berquist commented Oct 23, 2024

Closes #1482
Closes #1478

Rather than revert the PRs directly, all parsing and reference data is kept in atomic units. Converting from hartree to electronvolts or wavenumbers is done after parsing. For unit and regression tests, this conversion is done in the test functions to try and keep things generic, rather than applying the conversion to the reference data.

Part of reverting breaking changes on default
branch (cclib#1482)
Part of reverting breaking changes on default
branch (cclib#1482)
@berquist berquist added this to the v1.8.2 milestone Oct 23, 2024
Copy link

codecov bot commented Oct 23, 2024

Codecov Report

Attention: Patch coverage is 91.17647% with 3 lines in your changes missing coverage. Please review.

Project coverage is 81.41%. Comparing base (48cbef5) to head (8b0868f).
Report is 15 commits behind head on master.

Files with missing lines Patch % Lines
cclib/io/wfxwriter.py 0.00% 3 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1498      +/-   ##
==========================================
+ Coverage   81.38%   81.41%   +0.03%     
==========================================
  Files          73       73              
  Lines       14763    14789      +26     
==========================================
+ Hits        12015    12041      +26     
  Misses       2748     2748              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Part of reverting breaking changes on default
branch (cclib#1482)
@berquist
Copy link
Member Author

Back to trying to understand what the state of thermochemistry was before these changes. I'll check the units in the docs, but the results look like things where either implemented or fixed, not changed.

Before (7ceac1a)

HF/STO-3G

name version zpve enthalpy entropy freeenergy
0 DALTON 2020.0+66052b3af5ea7225e31178bf9a8b031913c72190 0.193502 nan nan nan
1 Gaussian 2003+e.1 0.193501 -379.575173 0.000145057856784 -379.618422
2 Gaussian 2016+a.3 0.193501 -379.575173 0.000145057856784 -379.618422
3 NWChem 6.0 0.193409 -379.575267894 0.000144981077974 -379.618494002
4 NWChem 7.0.2+b9985dfa 0.193409 -379.575268865 0.000144979484373 -379.618494498
5 NWChem 7.2.2+74936fb9 0.193409 -379.575268865 0.000144979484373 -379.618494498
6 ORCA 2.9.1 0.20747554 -379.56441872 0.000118843199732 -379.59985182
7 ORCA 3.0.3 0.19345679 -379.57521444 0.000143793124266 -379.61808636
8 ORCA 4.2.1 0.19345679 -379.57521444 0.000140988495724 -379.61725016
9 ORCA 5.0.4 0.19345678 -379.57521445 0.000140988495724 -379.61725018
10 QChem 5.3.1.dev0+trunk.34666 0.19350146094 0.202782595712 0.000145057570843 0.159533680965

Contents of data directory

name version zpve enthalpy entropy freeenergy
0 ADF 2007.1+200708191746 0.175884197049 nan 0.0416011560396 nan
1 ADF 2013.1+201309012319 0.176996530853 nan 0.0432052025783 nan
2 DALTON 2013.4+7abef2ada27562fe5e02849d6caeaa67c961732f 0.177062 nan nan nan
3 DALTON 2015.0+d34efb170c481236ad60c789dea90a4c857c6bab 0.177062 nan nan nan
4 GAMESS 8.0.1+8540 0.1935 -379.575178786 0.0001448982107 -379.618381321
5 GAMESS 2017.post1 0.177076 -381.863728193 0.000148759506983 -381.908081308
6 GAMESS 2018.post3 0.177076 -381.863728193 0.000148759506983 -381.908081308
7 GAMESSUK 7.0 0.1770069 nan nan nan
8 GAMESSUK 8.0+6248 0.1769506 nan nan nan
9 GAMESS 2018.post3 0.177076 -381.863728193 0.000148759506983 -381.908081308
10 Gaussian 2009+d.1 0.17714 -382.12129 0.000146372631226 -382.164931
11 Gaussian 2016+a.3 0.177132 -382.121307 0.000146261948684 -382.164915
12 Gaussian 2016+c.1 0.177132 -382.121307 0.000146258594667 -382.164914
13 Jaguar 7.0+207 0.17761484821 nan nan nan
14 Jaguar 8.3+13 0.177136767779 nan nan nan
15 Molcas 18.0+o180518.1800.dirty.9ead62c274ce0aa10a12879e848d75c9 0.1783 -382.11385 0.000134031442498 -382.153812
16 Molpro 2006.1 0.17704814 nan nan nan
17 Molpro 2012.1+c18f7d37f9f045f75d4f3096db241dde02ddca0a 0.17746077 nan nan nan
18 Molpro 2018.2+5ea806b172b3d20b18658228ad0b1c212d33fc17 0.17706097 nan nan nan
19 NWChem 7.0.2+b9985dfa 0.177031 -382.121387735 0.000146436036087 -382.165047639
20 NWChem 7.2.2+74936fb9 0.177031 -382.121387735 0.000146436036087 -382.165047639
21 ORCA 4.1.1 0.1770149 -381.86389179 0.000143852154956 -381.90678131
22 ORCA 4.2.0 0.1770149 -381.86389179 0.000143852154956 -381.90678131
23 ORCA 5.0.1 0.17701962 -381.86823907 0.00014384698977 -381.91112705
24 Psi4 1!1.2.1.dev0+head.406f4de 0.19167937 nan nan nan
25 Psi4 1!1.3.1.dev0+head.2ce1c29 0.19168284 nan nan nan
26 Psi4 1!1.7.dev0+head.6ce35a5 0.19168059 nan nan nan
27 QChem 5.1.2.dev0+trunk.29783 0.177294534322 0.187127055191 0.000146673482701 0.143396356323
28 QChem 5.4.1.dev0+trunk.37731 0.177294534322 0.187127055191 0.0001466718891 0.143396831456

After (110292d)

HF/STO-3G

name version zpve enthalpy entropy freeenergy
0 DALTON 2020.0+66052b3af5ea7225e31178bf9a8b031913c72190 0.193502 nan nan nan
1 Gaussian 2003+e.1 0.193501 -379.575173 0.000145057856784 -379.618422
2 Gaussian 2016+a.3 0.193501 -379.575173 0.000145057856784 -379.618422
3 NWChem 6.0 0.193409 -379.575267894 0.000144981077974 -379.618494002
4 NWChem 7.0.2+b9985dfa 0.193409 -379.575268865 0.000144979484373 -379.618494498
5 NWChem 7.2.2+74936fb9 0.193409 -379.575268865 0.000144979484373 -379.618494498
6 ORCA 2.9.1 0.20747554 -379.56441872 0.000118843199732 -379.59985182
7 ORCA 3.0.3 0.19345679 -379.57521444 0.000143793124266 -379.61808636
8 ORCA 4.2.1 0.19345679 -379.57521444 0.000140988495724 -379.61725016
9 ORCA 5.0.4 0.19345678 -379.57521445 0.000140988495724 -379.61725018
10 QChem 5.3.1.dev0+trunk.34666 0.19350146094 -379.575172811 0.000145057570843 -379.618421725

Contents of data directory

name version zpve enthalpy entropy freeenergy
0 ADF 2007.1+200708191746 0.175884197049 nan 0.000139530961058 nan
1 ADF 2013.1+201309012319 0.176996530853 nan 0.000144910959511 nan
2 DALTON 2013.4+7abef2ada27562fe5e02849d6caeaa67c961732f 0.177062 nan nan nan
3 DALTON 2015.0+d34efb170c481236ad60c789dea90a4c857c6bab 0.177062 nan nan nan
4 GAMESS 8.0.1+8540 0.1935 -379.575178786 0.0001448982107 -379.618381321
5 GAMESS 2017.post1 0.177076 -381.863728193 0.000148759506983 -381.908081308
6 GAMESS 2018.post3 0.177076 -381.863728193 0.000148759506983 -381.908081308
7 GAMESSUK 7.0 0.1770069 nan nan nan
8 GAMESSUK 8.0+6248 0.1769506 nan nan nan
9 GAMESS 2018.post3 0.177076 -381.863728193 0.000148759506983 -381.908081308
10 Gaussian 2009+d.1 0.17714 -382.12129 0.000146372631226 -382.164931
11 Gaussian 2016+a.3 0.177132 -382.121307 0.000146261948684 -382.164915
12 Gaussian 2016+c.1 0.177132 -382.121307 0.000146258594667 -382.164914
13 Jaguar 7.0+207 0.17761484821 -382.121011 0.000143519745456 -382.163802
14 Jaguar 8.3+13 0.177136767779 -382.121299 0.000146200183074 -382.164888
15 Molcas 18.0+o180518.1800.dirty.9ead62c274ce0aa10a12879e848d75c9 0.1783 -382.11385 0.000134031442498 -382.153812
16 Molpro 2006.1 0.17704814 nan nan nan
17 Molpro 2012.1+c18f7d37f9f045f75d4f3096db241dde02ddca0a 0.17746077 nan nan nan
18 Molpro 2018.2+5ea806b172b3d20b18658228ad0b1c212d33fc17 0.17706097 nan nan nan
19 NWChem 7.0.2+b9985dfa 0.177031 -382.121387735 0.000146436036087 -382.165047639
20 NWChem 7.2.2+74936fb9 0.177031 -382.121387735 0.000146436036087 -382.165047639
21 ORCA 4.1.1 0.1770149 -381.86389179 0.000143852154956 -381.90678131
22 ORCA 4.2.0 0.1770149 -381.86389179 0.000143852154956 -381.90678131
23 ORCA 5.0.1 0.17701962 -381.86823907 0.00014384698977 -381.91112705
24 Psi4 1!1.2.1.dev0+head.406f4de 0.19167937 -379.57027925 0.00013229827 -379.60972398
25 Psi4 1!1.3.1.dev0+head.2ce1c29 0.19168284 -379.57027653 0.00013229291 -379.60971966
26 Psi4 1!1.7.dev0+head.6ce35a5 0.19168059 -379.57027841 0.00013229523 -379.60972224
27 Psi4 1!1.7.dev0+head.6ce35a5 0.17709102 -382.12398393 0.00014674011 -382.16773449
28 QChem 5.1.2.dev0+trunk.29783 0.177294534322 -382.121147522 0.000146673482701 -382.164878221
29 QChem 5.4.1.dev0+trunk.37731 0.177294534322 -382.121147521 0.0001466718891 -382.164877745

@berquist
Copy link
Member Author

I checked the docs at the old revision, and all are given as hartree/particle (or hartree/particle*kelvin for entropy). So I think all the thermochemistry changes are not breaking: the units changing for Q-Chem are actually a bugfix.

@berquist berquist marked this pull request as ready for review October 24, 2024 16:53
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is all looking great and i like this approach. I notice in the unit tests we convert the reference data while in the regression tests we convert the ccdata units. Is the reason for this just convenience? I have no issue in with it, but just wanted to make sure I understood if there was another reason..

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In both cases, the ccData object has "convenience" units, and the conversion to a.u. happens outside of the object. For the unit tests it's easy to change because you can hide this in the generic test methods that are being called for each file, and in some cases there are a lot of different reference values, so it looks even cleaner. For the regression tests, the file-specific functions don't have any generic code in them because they aren't testing common things: just because two might check scfenergies doesn't mean that attribute will be checked in a similar way, and even if it was, the number of times a generic function would be called is low compared to the unit tests.

But, it is pretty ugly. There is a clever but gross hack we can do that may still be worth doing. For the regression tests, since the ccData is instantiated via the pytest fixture mechanism, we could do the conversion to a.u. for all files right at the source:

data = lfile.parse()
and get rid of the conversion calls in regression.py. They would go back what's before this PR, with an additional comment at the top of the file that these attributes as visible on the ccData object itself, are in atomic units.

What do you think? @amandadumi @shivupa

Copy link
Member

@amandadumi amandadumi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just had a question out of curiosity that I asked, but overall these changes look good to me and thanks for finding this nice solution.

@amandadumi amandadumi merged commit 6a6c66c into cclib:master Oct 30, 2024
27 checks passed
@berquist berquist deleted the 1482-revert-atomic-units-on-master branch October 31, 2024 23:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Revert atomic units changes on master branch Wrong description in the manual for moenergies
3 participants