-
Notifications
You must be signed in to change notification settings - Fork 23
Solution.from_preset: Add representative industrial wastewaters #317
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?
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #317 +/- ##
==========================================
+ Coverage 85.68% 86.43% +0.75%
==========================================
Files 10 14 +4
Lines 1607 1851 +244
Branches 285 320 +35
==========================================
+ Hits 1377 1600 +223
- Misses 194 207 +13
- Partials 36 44 +8 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Very excited to see these, @SuixiongTay ! Before we finalize, please also add the following:
|
YAML presets from WW project|
Hi @SuixiongTay , thanks for these updates. It looks like the automatic tests are getting stuck on |
|
Small edit - in the References in the docstring, please expand the preprint DOI into a full biobliographic reference (authors, title, preprint available on Research Square" |
Also, make sure to update your branch with |
Yes, it is running correctly for me locally, although the From the Cl log it looks like the Cl ran into a wall-time limit. One other work around would be to reduce the processing time either by 1) using their elemental compositions/proportions or 2) defining a threshold for the fully speciated ions which should help speed up the workflow. If the first approach makes sense, I can update the |
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.
Hi @SuixiongTay see these comments for some ideas about how to streamline the unit testing here
| # test invalid preset | ||
| with pytest.raises(FileNotFoundError): | ||
| Solution.from_preset("nonexistent_preset") | ||
| # test json as preset | ||
| json_preset = tmp_path / "test.json" | ||
| dumpfn(solution, json_preset) | ||
| solution_json = Solution.from_preset(tmp_path / "test") | ||
| assert isinstance(solution_json, Solution) | ||
| assert solution_json.temperature.to("degC") == ureg.Quantity(data["temperature"]) | ||
| assert solution_json.pressure == ureg.Quantity(data["pressure"]) | ||
| assert np.isclose(solution_json.pH, data["pH"], atol=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.
Since you are now parameterizing this entire test to run on multiple presets, this part of the test should be broken into a separate test, because it doesn't need to be run multiple times.
(these lines don't have anything to do with the specific preset file; they are testing behavior with unrecognized file names and custom files)
| for solute in solution._solutes: | ||
| assert solute in data["solutes"] |
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.
Instead of iterating, it will probably be much faster to compare the contents of the entire list at once, e.g.
assert set(solution._solutes) == set(data["solutes"]
This should ensure that every solute in the yaml is present in the solution
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.
Note that converting the list to a set is necessary b/c lists comparisons are ordered, but sets are not. An alternative would be to sort the lists first
| assert isinstance(solution, Solution) | ||
| assert solution.temperature.to("degC") == ureg.Quantity(data["temperature"]) | ||
| assert solution.pressure == ureg.Quantity(data["pressure"]) | ||
| assert np.isclose(solution.pH, data["pH"], atol=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.
Please change the atol here to 0.001; I can't think of a reason why we shouldn't be able to achieve that precision
| "seawater", | ||
| "ash", | ||
| "batt_mfg", | ||
| "batt_recycling", | ||
| "coal_washing", | ||
| "CRL", | ||
| "drilling", | ||
| "excavation", | ||
| "FGD", | ||
| "flotation", | ||
| "flue_gas", | ||
| "gasification", | ||
| "geothermal", | ||
| "leachate", | ||
| "mine_drainage", | ||
| "mine_tailings", | ||
| "plating", | ||
| "pw_conv", | ||
| "pw_unconv", | ||
| "refining", | ||
| "semiconductor", | ||
| "smelting", | ||
| "tanning", |
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 don't think it's necessary to test every preset; let's comment out all except for seawater and perhaps 2 others that have different compositions
Summary
Solution.from_preset. See https://www.researchsquare.com/article/rs-8743330/v2 for the analysis supporting these compositions.Todos: