Enable retrieving alkalinity from phreeqc2026 engine - #459
Conversation
|
Disclosure: I used Claude to help, but the changes are few and make sense to me. Sorry about lumping removal of |
rkingsbury
left a comment
There was a problem hiding this comment.
Thanks very much @jjstickel ; this is a helpful contribution! I'd like to proceed but with a few structural changes:
- If not too much trouble, can you split the
.gitignorechanges into another PR? That is a very helpful change; I'd just prefer to keep it separate - The changes to expose
ALKto the underlying PHREEQC solution (pyEQL/phreeqc/core.pyandsolution.py) are very helpful; I'm happy with those as-is - I'm not (yet) ready to make
get_alkalinityan engine method, so can you please revert the changes inpyEQL/engines.pyandpyEQL/solution.py? In general, I we try to use engine-specific methods only for those quantities that do not have an unambiguous definition, and rely onpyEQL.Solutionnative methods for everything else.
Regarding 3 - you have highlighted an important discrepancy between the pyEQL.Solution alkalinity and the underlying PHREEQC one which I want to instead treat as a bug in get_alkalinity. We recently implemented the weak acid-base definition of alkalinity (see #299 and #398 ) and perhaps that introduced a bug we weren't aware of. OR perhaps it isn't appropriate to rely on the Stumm and Morgan definition as the default (which is our current approach). Maybe it's more appropriate to use the proton condition as default.
I welcome additional thoughts!
|
Thanks for the feedback. I'll work on these as I have time (likely later this week or next week). |
|
FYI, I (actually Claude Code) took care of the |
aca095e to
9d5673d
Compare
I added a keyword argument to select between returning the alkalinity calculated by the engine vs. by pyEQL, and set it to be pyEQL by default. I work with geochemists who have been using PHREEQC for years and trust it a great deal. It is, and will continue to be, necessary for me in my work to report the alkalinity that PHREEQC calculates. Also, from the current bug in alkalinity, it seems that alkalinity calculations are not always straightforward, and so having a feature to calculate it a couple different ways could be useful. (I am not a geochemist, and so I am not able to suggest how to improve pyEQL's alkalinity calculation without performing some literature research). Does the keyword approach work for you? If this all gets sorted and everyone is convinced that pyEQL is doing the correct calculation [1], then the keyword argument could be removed in the future. [1] If the "correct" alkalinity calculation ends up being different from PHREEQC, even by a small fraction, it would be important to document why the calculations are different. |
Summary
Major changes:
Solution.engine.get_alkalinity()phreeqc2026engineTodos
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: