Add modified ADM1 model with P extension #948
Conversation
Codecov Report
@@ Coverage Diff @@
## main #948 +/- ##
==========================================
+ Coverage 95.62% 95.66% +0.04%
==========================================
Files 292 294 +2
Lines 27996 28497 +501
==========================================
+ Hits 26770 27262 +492
- Misses 1226 1235 +9
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
watertap/property_models/anaerobic_digestion/adm1_properties_P.py
Outdated
Show resolved
Hide resolved
|
I've tried re-running the MacOS setup test, but it still fails. Is it fine if this test is just skipped? |
| return self.conc_mol_Mg == self.conc_mass_comp_ref["X_PP"] / ( | ||
| 300.41 * pyo.units.kg / pyo.units.kmol |
There was a problem hiding this comment.
This seems to be a copy of the previous constraint.
There was a problem hiding this comment.
Thx for catching this - the expressions are equivalent, but I did forget to replace conc_mol_Mg with conc_mol_K
There was a problem hiding this comment.
What molecule does that molecular weight correspond to? I figure X_PP represents polyphosphates but unsure what the MW assumption specifically ties to.
There was a problem hiding this comment.
Screenshot from Excel located here in Box: https://app.box.com/file/1153207546703
So, I think it depends on what is failing in the test. Originally, it was just a matter of absolute tolerance. However, after latest changes, it appears to be more of a scaling issue (I suspect) that now throws off the AD unit's initialization. It'd be better to try to resolve that before we resort to skipping. That said, it'd be helpful for a Mac user to tinker with this. @bknueven could you take a look at this? I am sure you will find that there is substantial room for improvement in scaling and initialization for these models, but for now, we're just looking to hit the bare minimum requirement to get an initial implementation passing tests and merged. Following that, we'd have subsequent PRs to address scaling and initialization. |
|
Another question/comment that I want @MarcusHolly and @luohezhiming to think about ahead of our meeting today: our slides showing ADM1->ASM2D and ASM2D ->ADM1 give a table that lists struvite (MgNH4PO4) as well as calcium phosphate (Ca3(PO4)2), both of which will be key for our 2336 team collaboration. It seems that the current implementation hasn't accounted for those species yet (but I may have missed that). If so, how much more would it take to incorporate those species? Think about it, and let's discuss later today =) |
The quick answer is that we don't currently account for these species. However, they are accounted for in the Precipitation Extension in the Flores-Alsina paper: https://app.box.com/file/1153202083720?s=he1whq9a66fvxj8y39puqsqijzuy7sbr I haven't looked too closely at how much effort this would require, so I'll try to look into it before the meeting. |
|
For this PR we should just mark the failing tests with |
watertap/property_models/anaerobic_digestion/tests/test_modified_adm1_reaction.py
Show resolved
Hide resolved
|
@adam-a-a I've avoided changing some of the pytest marks from component to unit because this test file follows the same conventions as other similar test files for adm1, asm1, and asm2d. So if these tests really are incorrectly marked, then maybe we/I should just fix them all in one-go (in a separate PR) rather than just updating this test file. |
* Add modified ADM1 model with P extension * Add reference * Add more sig figs * R10-R11 * R12-R18 * add stoichiometry * add details for modified ADM1 * Correct f_xi_xb value * Fix R3 S_IP stoich * Remove X_IP terms from stoich * Remove X_c terms from stoich * update Z_h2s * Rename packages * add tests * ADD TESTS * Update parameter values and test file reference * Update test file * Add to-do note * add P extension * Add P extension * Add S_IC, S_IP, and S_IN for R19-R25 * Fix test file failures * Remove completed to-do tasks * delete unused parameters * Resolve completed to-do items * Resolve some test failures * Remove X_c from thermo test * Solve InconsistentUnitError by making Y_SO4 dimensionless * Temporarily fixes DOF failure * Undo last change * Replace instances of f_si_xb with 0 * Make Z_h2s a parameter * Undo last change and input initial rxn rate values for R19-25 * Try initialization utility tools * Update test feed conditions and replaces X_K & X_Mg with S_K & S_Mg * Add the values for the remaining rxn rates * Correct initial values for rxn rates * Expand rxn rate bounds and add to-do list for test file * improve scaling on adm1+P reactions and remove bounds on rate_reaction * add stream table to AD unit and additional solve to initialization * fix modified adm1 tests, add scale factors and todo notes to AD unit * make sure temporary scaling in AD unit is only used for resolving modified ADM1 until real fix is implemented * replace relative tolerances with absolute tolerances--for results expected to be ~0 * missed an abs tolerance * Clean up to-do items and comments * Track Mg and K ions * Fix initialization failure associated with tracking Mg & K ions * Address Adam's comment * Add requires_idaes_solver marks * Only mark the failing initialization test w/requires_idaes_solver * Add require_idaes_solver to all failing tests --------- Co-authored-by: MarcusHolly <marcus.holly@keylogic.com> Co-authored-by: adam-a-a <aatia@keylogic.com> (cherry picked from commit 7d14f22)
Fixes/Resolves:
(replace this with the issue # fixed or resolved, if no issue exists then a brief statement of what this PR does)
Summary/Motivation:
Changes proposed in this PR:
Legal Acknowledgement
By contributing to this software project, I agree to the following terms and conditions for my contribution: