-
Notifications
You must be signed in to change notification settings - Fork 21
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
pymatgen>=2024.9.10 updated after bugfix and conflict resolve #188
Conversation
Looks like it can't find the newest pymatgen version 2024.9.10
|
Ah, I guess that's because pymatgen just dropped support for python 3.9 (I had missed that in the CHANGELOG). So the python 3.9 unit test can't install that version. Per the README file, we try to follow the I know this expands the scope of the PR somewhat, but if you're up for it, feel free to take a stab at dropping python 3.9. You would need to update the requirement in The other option is to add a conditional requirement in |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #188 +/- ##
==========================================
+ Coverage 75.84% 81.96% +6.12%
==========================================
Files 9 9
Lines 1486 1486
Branches 322 322
==========================================
+ Hits 1127 1218 +91
+ Misses 315 224 -91
Partials 44 44 ☔ View full report in Codecov by Sentry. |
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.
Nice work @abhardwaj73 , thanks for this. In addition to the one comment above, it looks like the ubuntu tests started to fail because of a floating point precision error:
FAILED tests/test_phreeqc.py::test_equilibrate - AssertionError: assert False
+ where False = <function isclose at 0x7f623e12a070>(0.0010000227561117677, 0.001)
+ where <function isclose at 0x7f623e12a070> = np.isclose
+ and 0.0010000227561117677 = <Quantity(0.0010000227561117677, 'mole')>.magnitude
+ where <Quantity(0.0010000227561117677, 'mole')> = get_total_amount('C(4)', 'mol')
+ where get_total_amount = <pyEQL.solution.Solution object at 0x7f6217847b50>.get_total_amount
================== 1 failed, 130 passed, 3 xfailed in 55.68s ===================
As you can see, the test uses np.isclose
to compare the actual and expected output, and this one happens to use the default tolerance of 1e-08. The actual and expected outputs differ by just barely more than that, so it fails.
I've manually re-run that test to make sure this wasn't just a quirk of a single worker (computer), but I'm also fine with relaxing this tolerance to atol=1e-7
or atol=1e-6
, because that is still much smaller than the concentration we're testing.
It's not clear to me exactly why this started now, but it must be related to the fact that we're now using python 3.10 instead of python 3.9 for the ubuntu tests. |
Adding atol=1e-7 in the affected C(4) case helped pass the ubuntu test. All other tests passed except for the codecov/project test (Project coverage is 81.96% (-1.22% from the last commit)). Currently, pyEQL requires |
Thanks @abhardwaj73 ! Closes #158 |
Summary
Major changes:
pymatgen
version to 2024.9.10 after bugfixpython 3.9
followingnumpy
policyIssue #158