-
Notifications
You must be signed in to change notification settings - Fork 23
Fix incorrect reduction of dimers/polymers in standardize_formula #309
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
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #309 +/- ##
===========================================
+ Coverage 74.55% 84.81% +10.25%
===========================================
Files 10 10
Lines 1521 1521
Branches 265 264 -1
===========================================
+ Hits 1134 1290 +156
+ Misses 352 199 -153
+ Partials 35 32 -3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Ooof, good catch @vineetbansal . I have no idea how to interpret (CO2)2. I suppose for the time being we should add an ad-hoc patch to |
src/pyEQL/engines.py
Outdated
| # --- Code to demonstrate bug --- | ||
| # The standardized formula for (CO2)2 is still CO2, so this amount | ||
| # (negligible) ends up overwriting the original CO2 amount. | ||
| # We simply get lucky in the loop above that (CO2)2 is encountered | ||
| # before CO2 in the old wrapper, but we force surfacing the bug here. | ||
| problematic_keys = ("(CO2)2",) | ||
| for k in problematic_keys: | ||
| if k in self.ppsol.species_moles: | ||
| solution.components[k] = self.ppsol.species_moles[k] | ||
|
|
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 think this can be removed, now that the fix is in place
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.
Thanks! I just wanted to see if the tests pass before removing this. Hopefully the fix makes sense.
This is most likely a bug that we haven't encountered yet, simply because we're getting lucky in the order of the keys (representing species->amount mapping) in the current wrapper.
The tests that should fail in this PR is
test_phreeqc.py::test_equilibrate_logC_pH_carbonate_8_3(among others).The issue seems to be that for a simple solution like this:
corresponding to the string:
In addition to
CO2,phreeqcreports a tiny amount of(CO2)2(whatever that represents, not sure..). The following is the output from phreeqc UI, and is consistent with what the old (and our new) wrappers return.The
standardize_formulafunction standardizes(CO2)2asCO2, and can possibly end up overwriting theCO2amount with the(CO2)2amount (which is tiny), in the following logic in.equilibrate():We simply get lucky that
(CO2)2is consistenly reported beforeCO2by the current wrapper, so we end up with the correct amount ofCO2. In the new wrappers, the order of the keys in the dict is reversed (for whatever reason, but most probably because we're building the dictionary in the same order as the output of phreeqc UI indicates), so we do encounter this bug.The attached snippet simply surfaces the bug without fixing it. I'm not sure of a solution without the background on what it means/entails.