-
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?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,32 @@ | ||
| import warnings | ||
| warnings.filterwarnings("ignore", category=RuntimeWarning) | ||
|
|
||
| import textwrap | ||
| import pytest | ||
|
|
||
| from pyEQL import Solution | ||
|
|
||
| def test_from_phreeqc_minimal_parser(tmp_path): | ||
| """Smoke-test our new from_phreeqc() on a tiny SOLUTION block.""" | ||
| pqi = tmp_path / "sample.pqi" | ||
| pqi.write_text(textwrap.dedent(""" | ||
| SOLUTION foo | ||
| pH 7.5 | ||
| temp 30 | ||
| Ca+2 0.01 | ||
| SO4-2 0.01 | ||
|
Comment on lines
+16
to
+17
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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: 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 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. |
||
| END | ||
| """)) | ||
| sol = Solution.from_phreeqc(str(pqi)) | ||
| assert sol.pH == pytest.approx(7.5, rel=1e-6) | ||
| assert sol.temperature.magnitude == pytest.approx(303.15, rel=1e-6) | ||
| assert sol.get_amount("Ca+2", "mol/L").magnitude == pytest.approx(0.01, rel=1e-6) | ||
| assert sol.get_amount("SO4-2", "mol/L").magnitude == pytest.approx(0.01, rel=1e-6) | ||
|
|
||
| def test_from_phreeqc_missing_block_raises(tmp_path): | ||
| """If there is no SOLUTION block, we should get a ValueError.""" | ||
| pqi = tmp_path / "empty.pqi" | ||
| pqi.write_text("THIS FILE HAS NO SOLUTION BLOCK\n") | ||
| with pytest.raises(ValueError): | ||
| Solution.from_phreeqc(str(pqi)) | ||
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 ofmol/L. But it would be even better to parse theunitsandwaterkeywords as well so that this method can detect when users specify different units. Seehttps://wwwbrr.cr.usgs.gov/projects/GWC_coupled/phreeqc/html/final-56.html#89789
for a description of how they work.