-
Notifications
You must be signed in to change notification settings - Fork 23
add Solution.from_phreeqc() + tests #260
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
base: main
Are you sure you want to change the base?
add Solution.from_phreeqc() + tests #260
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #260 +/- ##
==========================================
+ Coverage 81.79% 81.97% +0.17%
==========================================
Files 9 9
Lines 1494 1531 +37
Branches 257 266 +9
==========================================
+ Hits 1222 1255 +33
- Misses 226 229 +3
- Partials 46 47 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
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.
@NikhilDhruv , thank you for the great work on this! Overall it looks good, and thank you especially for writing unit tests! I've made a comment about units and the input file format, and called your attention to a related bug #229 that needs to be addressed in order to work properly.
If you're not familiar with it, I encourage you to download and try the graphical PHREEQC interface (https://wwwbrr.cr.usgs.gov/projects/GWC_coupled/phreeqci/) to test out your input files.
| def from_phreeqc(cls, path_to_pqi: str) -> "Solution": | ||
| """ | ||
| Parse a PHREEQC .pqi input file and pull out only the SOLUTION block, | ||
| extracting solute amounts (assumed mol/L), plus pH, pE, and temperature if present. |
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 believe the default units (if the user doesn't specify anything) are mmol/kgw, so at a minimum you should assume that instead of mol/L. But it would be even better to parse the units and water keywords as well so that this method can detect when users specify different units. See
https://wwwbrr.cr.usgs.gov/projects/GWC_coupled/phreeqc/html/final-56.html#89789
for a description of how they work.
| Ca+2 0.01 | ||
| SO4-2 0.01 |
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.
One challenge with this is that PHREEQC inputs are normally specified by elements rather than specific ionic species. I have not tested (yet), but I'm pretty sure that the simple input file you create for this test is not actually a valid PHREEQC input, because it lists individual species rather than elements.
A typical PHREEQC concentration input would like, for example:
Ca 80.
S(6) 96. as SO4
N(5) N(3) 14. as N
See https://wwwbrr.cr.usgs.gov/projects/GWC_coupled/phreeqc/html/final-56.html#89789 for other examples.
In the above, the number in parentheses indicates the oxidation state. Note that some elements list "as" which tells you what species they should be interpreted as, but not all. For example "Ca" is just "Ca". Would it be possible to detect "as" keywords and then use whatever species comes after the "as"?
For elements that do not have "as", PHREEQC usually assigns a reasonable default species such as interpreting Ca as Ca+2). However, there is a related bug - #229 that we need to address when a bare element like Ca is used as input to pyEQL.
Can you investigate how to fix that bug? It could be done in a separate PR if you want, but it's necessary in order to get PHREEQC input to parse correctly.
|
Closes #260 |
|
Also, please add a line to the CHANGELOG documenting what you've added here! |
|
I'm tagging @SuixiongTay who is a member of our group and has experience with PHREEQC as well |
|
@rkingsbury Thanks so much for the thorough review and for flagging the units/defaults issue and bug #229—I’ll dive into parsing the mmol/kgw defaults (and any “as” keywords) and update the method accordingly. I’ll also add a line to the CHANGELOG and download the PHREEQC GUI to test my input files end-to-end. Hi @SuixiongTay – I’m going to start experimenting with the graphical interface and the units parsing logic; I’d really appreciate any tips or pointers you have from your PHREEQC experience. Thanks again! |
|
Hi @NikhilDhruv, thanks for digging into this! I wanted to share some of my favorite resources that helped me when I was learning PHREEQC (both setting up calculations, plotting results, etc.)
Personally, I'd recommend starting from the UTAustin guide/manual for a broader overview, then referencing the official manual. Happy to work with you as you explore this! Please feel free to reach out anytime with any questions or ideas. :) |
Hi Professor Kingsbury,
I’ve added a new class method, Solution.from_phreeqc(), which can read a PHREEQC‐style .pqi file, locate its SOLUTION block, and automatically extract pH, pE, temperature, and all solute concentrations—raising a clear ValueError if no block is found. I also included a dedicated test suite (tests/test_parser_from_phreeqc.py) covering both a minimal valid SOLUTION block and the missing‐block error case to ensure robust, maintainable parsing functionality. Let me know if you’d like any further adjustments!