Skip to content

Conversation

@vineetbansal
Copy link
Collaborator

@vineetbansal vineetbansal commented Nov 21, 2025

Code and tests for PHREEQC's phase equilibrium calculations.

This PR supersedes PR 286

Not done yet - @SuixiongTay was planning to add some tests, and I will as well.

Checklist

  • Google format doc strings added.
  • Code linted with ruff. (For guidance in fixing rule violates, see rule list)
  • Type annotations included. Check with mypy.
  • Tests added for new features/fixes.
  • I have run the tests locally and they passed.

Tip: Install pre-commit hooks to auto-check types and linting before every commit:

pip install -U pre-commit
pre-commit install

@codecov
Copy link

codecov bot commented Nov 21, 2025

Codecov Report

❌ Patch coverage is 92.30769% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 84.77%. Comparing base (c630c50) to head (e357b71).
⚠️ Report is 29 commits behind head on main.

Files with missing lines Patch % Lines
src/pyEQL/engines.py 91.30% 1 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #292      +/-   ##
==========================================
+ Coverage   84.21%   84.77%   +0.55%     
==========================================
  Files          10       10              
  Lines        1495     1517      +22     
  Branches      261      263       +2     
==========================================
+ Hits         1259     1286      +27     
+ Misses        203      199       -4     
+ Partials       33       32       -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@vineetbansal vineetbansal changed the title Phase-Equilibrium Calculations WIP - Phase-Equilibrium Calculations Nov 21, 2025
@rkingsbury rkingsbury mentioned this pull request Nov 24, 2025
5 tasks
@SuixiongTay SuixiongTay mentioned this pull request Nov 24, 2025
5 tasks
Sui Xiong Tay and others added 2 commits November 24, 2025 16:27
@vineetbansal
Copy link
Collaborator Author

Removed all tests. @SuixiongTay - please add tests as you see fit. As we discussed earlier, the codecov/patch action will tell you if all the newly introduced code lines have been covered.

@vineetbansal
Copy link
Collaborator Author

Thanks @SuixiongTay. @rkingsbury - can you wait till Friday (12/12) before you look at this? I'm meeting with @SuixiongTay tomorrow and we may add one more commit with some minor tweaks.

Copy link
Member

@rkingsbury rkingsbury left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks everyone! Great work overall. I've requested a series of minor changes (mostly docstrings, comments, and small tweaks) mostly to support future maintain-ability and clarity. I'll merge as soon as addressed.

@vineetbansal
Copy link
Collaborator Author

@SuixiongTay, @YitongPan1 - I've added a commit addressing all conversations above, except these 3:

Please add N2 here. I know it doesn't really dissolve to any appreciable extent in most cases, but we should have it here to avoid unexpected behavior

Also, being picky here, is -3.5 the most up to date partial pressure of CO2? It has changed in our lifetimes, I believe the current atmospheric conc. is close to 400 ppm? Please include a reference for the values you used in the comments.

Please add one more test test_equilbrate_with_atm that includes only atmosphere=True (no other gases). Then check that the concentrations of all three - CO2, O2, and N2 are as expected. (Currently the O2 concentration is not tested anywhere).

Addressing the first may change some test values - I think we should be okay adding a commit with the changed atmospheric composition, then another commit that fixes failing tests (if any) because of that change.

rkingsbury
rkingsbury previously approved these changes Dec 16, 2025
@vineetbansal
Copy link
Collaborator Author

Thanks @SuixiongTay - your final edits make sense. I've added one more commit, mostly with a wording change that Ryan wanted, and some additional comments on atmospheric composition.

Regarding the differences between Phreeqc UI and Phreeqc Wrapper, I suspect we can make them match exactly by passing in some kind of precision setting, but I'm not sure. I'll make a note of this (if it works, it will be a useful debugging feature in the new wrappers anyway).

@rkingsbury rkingsbury changed the title WIP - Phase-Equilibrium Calculations enable solid-liquid and gas-liquid equilibrium calculations in equilibrate() Dec 17, 2025
@rkingsbury rkingsbury merged commit d75cf06 into main Dec 17, 2025
16 of 17 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants