-
Notifications
You must be signed in to change notification settings - Fork 23
Vb/issue186a #286
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
Vb/issue186a #286
Conversation
|
All newly introduced tests here translate to the IPhreeqc string: I'm hoping that someone (@SuixiongTay ?) can fill in the correct values for these tests by running the above script in IPhreeqc along with the correct variation (should be different for each case) on the I've chosen CuO simply because ChatGPT tells me that its a compound likely to equilibrate to different |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #286 +/- ##
==========================================
+ Coverage 84.89% 85.08% +0.19%
==========================================
Files 9 10 +1
Lines 1476 1495 +19
Branches 257 261 +4
==========================================
+ Hits 1253 1272 +19
Misses 193 193
Partials 30 30 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
For reference, we plan to define |
I think this will be fine. The only other non-trivial components of the atmosphere are N2 and Argon, and I'm fairly certain that neither of those partitions into water to any meaningful degree. I found a compilation of Henry's law constants, which shows that the Kh values for Ar and N2 actually appear to be on a similar order of magnitude to those of O2. @SuixiongTay - when you're running test simulations, can you include a case where N2 and Ar are included in I think in the end we will probably just stick with O2 and CO2, but it'll be good to double check. |
tests/test_phreeqc.py
Outdated
| assert np.isclose(s6_Ca.charge_balance, 0, atol=1e-8) | ||
|
|
||
|
|
||
| def test_equilibrate_vacuum(): |
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.
let's call this test test_equilibrate_liquid b/c technically it represents a closed container (no contact with gas/solid/vacuum)
tests/test_phreeqc.py
Outdated
|
|
||
|
|
||
| def test_equilibrate_vacuum(): | ||
| solution = Solution([["Cu+2", "4 mol/L"], ["O-2", "4 mol/L"]], volume="2 L", engine="phreeqc") | ||
| solution.equilibrate() | ||
| assert solution.get_total_amount("Cu", "mol").magnitude == pytest.approx(None) # TODO: fill-in | ||
|
|
||
|
|
||
| def test_equilibrate_with_atm(): | ||
| solution = Solution([["Cu+2", "4 mol/L"], ["O-2", "4 mol/L"]], volume="2 L", engine="phreeqc") | ||
| solution.equilibrate(atmosphere=True) | ||
| assert solution.get_total_amount("Cu", "mol").magnitude == pytest.approx(None) # TODO: fill-in | ||
|
|
||
|
|
||
| def test_equilibrate_with_co2_pp(): | ||
| # Specify partial pressure of equilibrium gas(es) directly, as log10_<partial_pressure> values. | ||
| solution = Solution([["Cu+2", "4 mol/L"], ["O-2", "4 mol/L"]], volume="2 L", engine="phreeqc") | ||
| solution.equilibrate(gases={"CO2": -2}) | ||
| assert solution.get_total_amount("Cu", "mol").magnitude == pytest.approx(None) # TODO: fill-in | ||
|
|
||
|
|
||
| def test_equilibrate_with_co2_pp_atm(): | ||
| # Specify partial pressure of equilibrium gas(es) directly, but in some recognizable units. | ||
| solution = Solution([["Cu+2", "4 mol/L"], ["O-2", "4 mol/L"]], volume="2 L", engine="phreeqc") | ||
| solution.equilibrate(gases={"CO2": "0.01 atm"}) | ||
| assert solution.get_total_amount("Cu", "mol").magnitude == pytest.approx(None) # TODO: fill-in | ||
|
|
||
|
|
||
| def test_equilibrate_with_calcite(): | ||
| solution = Solution([["Cu+2", "4 mol/L"], ["O-2", "4 mol/L"]], volume="2 L", engine="phreeqc") | ||
| solution.equilibrate(solids=["Calcite"]) | ||
| assert solution.get_total_amount("Cu", "mol").magnitude == pytest.approx(None) # TODO: fill-in | ||
|
|
||
|
|
||
| def test_equilibrate_with_calcite_and_atm(): | ||
| solution = Solution([["Cu+2", "4 mol/L"], ["O-2", "4 mol/L"]], volume="2 L", engine="phreeqc") | ||
| solution.equilibrate(atmosphere=True, solids=["Calcite"]) | ||
| assert solution.get_total_amount("Cu", "mol").magnitude == pytest.approx(None) # TODO: fill-in |
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 set of tests look great (pending adding the right values)
One more we should add - we'll want to handle collisions between atmosphere=True and gases={<something>} kwargs. If a user specifies both, we could either
-
Combine the contents of the
atmospheredict with whatever the user supplied, overwriting any duplicated keys (e.g.,atmosphere=True, gases={"CO2": "0.01 atm"}should overwrite{"CO2": -3.5}in the call to IPHREEQC). If a key gets overwritten, a warning or INFO message should be logged. -
Ignore the
atmospherekwarg and use only what the user specified, while logging a WARNING
I'm in favor of option 1 b/c I can imagine many users might want to simulate "the atmosphere plus some trace impurity gas", and option 1 would make that easy to do. But I could be convinced for option 2 as well. @SuixiongTay @YitongPan1 what do y'all think?
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.
Makes sense to me - I've implemented the "atmosphere plus some trace impurity gas.." approach in the upcoming PR.
|
Hi @vineetbansal , below are the ion species distribution after equilibrium from PHREEQC 1.CuO test casePHREEQC input: PHREEQC output: 2.Calcite test caseA simpler test case would be by equilibrating water with calcite (pKsp= 8.48) and CO2 at pH 8.4: PHREEQC input: With the ion species distribution after equilibrium below: PHREEQC output: |
Hi @rkingsbury , here are the outputs after including N2 for equilibration. The current PHREEQC database does not include aqueous complexes for Ar. I believe including O2, N2, and CO2 should be sufficient for the PHREEQC wrapper PHREEQC input: PHREEQC output: |
Sounds good to me! Thanks for checking. Yeah, I see that aqueous |
|
@rkingsbury, @SuixiongTay - I've filled in the values in the tests by confirming that the Phreeqc strings generated in all cases "make intuitive sense", and added the test about aqueous N2 as mentioned in the thread above. |
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 looked to me like the natural place to put the newly introduced constants. This makes it a submodule of pyEQL along with other data files.
|
Hi all, here are some additional test cases :) |
|
Hi @YitongPan1 - thanks for the additional tests. However, I think there are some potential problems in interpretation, which may be on my end (but I think are unrelated to this PR in any case): I believe the correct call as per pyEQL convention should be: Further, for 1L of water, the amount of H2O turns out to be ~55 moles and the amount of H+ is thus ~110 moles. I'd be interested to know why you expect Also, running these test cases locally against the |
Actually no, @YitongPan1's version is the correct one. One way to check is to pass the formula ( The semantic difference between
|
Actually, nevermind, the volume is already account for by the |
@vineetbansal the amounts of H2O and H+ are different actually. These refer, respectively, to the amount of pure water and the amount of protons (the basis for pH). I think the source of the confusion might be related to the semantic difference between |
|
@rkingsbury - thanks for the clarification. Sure enough, from the docstrings: The following asserts work with the In any case, @YitongPan1's |
That's strange. When I'm on an up to date @vineetbansal can you post your output and @YitongPan1 can you make sure you're running these on the latest |
|
@rkingsbury - your REPL snippet is expected, and what I'm getting as well. What I meant was that the code that @YitongPan1 is using in her asserts (in her "additional test cases" blurb) is failing, (as expected after our discussion above): Since the syntax in her suggested tests is painstakingly correct, I was just wondering if that code is/was working in some branch, but is not working on |
OK good!
I see now - sorry I missed this before. I could have sworn the first |
|
Closing this in favor of PR#292. We can refer back to the discussions here if needed. |
Summary
Major changes:
Added some currently failing tests for PHREEQC's solid-liquid equilibrium calculations. The
Nonevalues in all the newly-introduced tests (with the#TODO: fill-inmarker) need to be filled in before I can add a commit that adds the desired functionality.This PR builds on top of PR #285 so that we don't get flagged on failing CI tests that have nothing to do with the changes proposed here. I'm happy to rebase this PR based on suggestions to PR #285 (if any).
Todos
If this is work in progress, what else needs to be done?
Nones, preferably by running these cases through a standalone PhreeqC workflow.Checklist
ruff. (For guidance in fixing rule violates, see rule list)mypy.Tip: Install
pre-commithooks to auto-check types and linting before every commit: