-
Notifications
You must be signed in to change notification settings - Fork 23
PHREEQC pytest
#290
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
PHREEQC pytest
#290
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #290 +/- ##
==========================================
+ Coverage 84.89% 84.94% +0.05%
==========================================
Files 9 10 +1
Lines 1476 1495 +19
Branches 257 261 +4
==========================================
+ Hits 1253 1270 +17
- Misses 193 194 +1
- Partials 30 31 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
@SuixiongTay - thanks! With this PR though, there is no logical connection (as far as git is concerned) between what's been done so far, and what I may intend to do in the next couple of days. To coordinate our efforts, can you resubmit this PR with the target set to my fork and branch (vineetbansal:vb/issue186a) ? That way your changes can feed into mine, and our combined changes can feed into the already existing PR. |
rkingsbury
left a comment
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 @SuixiongTay , these tests look good. Please make the changes requested and also add one more test where you set both atmosphere=True and also explicitly set a N2 or CO2 concentration with gases. We want to test what happens when a user supplies both (I believe we decided the contents of gases should override atmosphere and a warning should be logged.
| s6_Ca.equilibrate() | ||
| assert s6_Ca.get_total_amount("Ca", "mol").magnitude != initial_Ca | ||
| assert np.isclose(s6_Ca.charge_balance, 0, atol=1e-8) | ||
|
|
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.
Tests look good to me. Can you please add a 1 line comment above each of the assert statements (in tests 2-4) explaining how you arrived at the correct test value?
Something like "p_N2 = 0.78, Henry's law = XXX, dissolved N2 = YYYY"
|
Finally, I'm going to close this and request that you integrate the changes into #292 by pushing to @vineetbansal branch. Thanks! |
Summary
This PR adds 4 new test functions to
tests/test_phreeqc.py:test_equilibrate_ca_co3_liquid()test_equilibrate_ca_co3_with_atm()test_equilibrate_ca_co3_with_n2_pp()test_equilibrate_calcite_with_co2()Todos
pyEQLvsPHREEQCdensity