-
Notifications
You must be signed in to change notification settings - Fork 23
Phreeqc wrapper #291
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 wrapper #291
Conversation
package, installed with the "phreeqc" extra.
f9da6a6 to
5a791de
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #291 +/- ##
==========================================
- Coverage 84.89% 84.46% -0.44%
==========================================
Files 9 9
Lines 1476 1493 +17
Branches 257 258 +1
==========================================
+ Hits 1253 1261 +8
- Misses 193 201 +8
- Partials 30 31 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
…hon incompatibility)
|
@rkingsbury - this is ready to be reviewed from my end. The wrapper is a work in progress of course, and will remain so for a few iterations, but since there is quite a bit of mechanical stuff and glue code to review here, we can do this in stages - merge now with the assurance that nothing breaks with this PR, and develop the wrapper in functionality over the next few PRs. This would be my recommendation. Or we can do a single PR for a "good-enough" phreeqc wrapper for pyEQL purposes. Here's what's happening:
Rationale for this two-packages-in-a-repo:
That said, tests for I suppose the choice depends on whether Some other points:
|
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 for this @vineetbansal . I appreciate the detailed rationale you provided to help me understand what's happening here!
I'm happy to merge now. Here are a list of items / comments I want to follow up on as we continue to refine the new wrapper:
- Will it generally be necessary to include the entire contents of IPHREEQC in our repo, or is that just a convenience for the time being? I'm thinking about the implications for future updates as new versions of IPHREEQC come out.
- I am 99+% sure that IPHREEQC is public domain code and hence should be no problem to include in our repo, which is LGPL licensed, but I want to verify this. I'm having trouble finding an explicit license declaration anywhere on the USGS websites.
- Great point re: source vs. binary wheels. I had never thought of that. There is definitely value in pyEQL without the PHREEQC wrapper
- I like keeping both
pyEQLandpyEQL-phreeqcin the same repo to facilitate development and maintenance (one fewer repo to maintain is a good thing), even if we release the packages separately - Re: package name - I'm not sure yet what the ideal strategy is; I think
pyEQL-phreeqcis a very sensible place to start rymndhng/release-on-push-action@v0.28.0is just an action I copied from another repo that enables "release on tag". I'd love to talk to you about a better or cleaner way to accomplish this (perhaps using an "official" github action instead)- Let's circle back to how the
enginekwarg will activate the new wrapper. My intention is for this new wrapper to completely replacephreeqpythonand I don't plan to maintain that support for very long. So I'd like to see your work become the default in thenativeandphreeqcengines, but we should also have a transitional period over a few releases where a separateengineargument is used to activate your wrapper.
WIP.
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: