-
Notifications
You must be signed in to change notification settings - Fork 174
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
Revert breaking API changes on master branch #1498
Conversation
Part of reverting breaking changes on default branch (cclib#1482)
Part of reverting breaking changes on default branch (cclib#1482)
Codecov ReportAttention: Patch coverage is
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. |
Part of reverting breaking changes on default branch (cclib#1482)
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
Contents of
|
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 |
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. |
There was a problem hiding this comment.
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..
There was a problem hiding this comment.
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:
Line 203 in 6a6c66c
data = lfile.parse() |
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
There was a problem hiding this 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.
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.