Add Modified ADM1 flowsheet #1469
Conversation
watertap/flowsheets/anaerobic_digester/modified_ADM1_flowsheet.py
Outdated
Show resolved
Hide resolved
watertap/flowsheets/anaerobic_digester/modified_ADM1_flowsheet.py
Outdated
Show resolved
Hide resolved
watertap/flowsheets/anaerobic_digester/modified_ADM1_flowsheet.py
Outdated
Show resolved
Hide resolved
watertap/flowsheets/anaerobic_digester/modified_ADM1_flowsheet.py
Outdated
Show resolved
Hide resolved
watertap/flowsheets/full_water_resource_recovery_facility/BSM2_P_extension.py
Show resolved
Hide resolved
watertap/flowsheets/full_water_resource_recovery_facility/BSM2_P_extension.py
Outdated
Show resolved
Hide resolved
watertap/flowsheets/full_water_resource_recovery_facility/BSM2_P_extension.py
Outdated
Show resolved
Hide resolved
watertap/flowsheets/full_water_resource_recovery_facility/test/test_BSM2_P_extension.py
Outdated
Show resolved
Hide resolved
| # if degrees_of_freedom(self) != 0: | ||
| # raise InitializationError( | ||
| # f"{self.name} degrees of freedom were not 0 at the beginning " | ||
| # f"of initialization. DoF = {degrees_of_freedom(self)}" | ||
| # ) |
There was a problem hiding this comment.
Why is this line being commented out?
There was a problem hiding this comment.
This one is commented out for passing the initialization, if we include this check, then after propagate_state, they still raise the error. Maybe @agarciadiego has an idea on how to rewrite this check
There was a problem hiding this comment.
Looking at other unit models, it seems like this DOF check is before the initialization in some cases and after the initialization in others (most unit models actually don't include this check at all). I'm not sure why one option should be chosen over the other, but if we move this check to after the initialization, it seems like everything passes on my end.
There was a problem hiding this comment.
It's prob best if this commented code is moved to the end of the initialization routine or just deleted altogether
There was a problem hiding this comment.
Given this is an initialization routine, it is important to check that the DoF are 0 before starting (or at least what you expect them to be). If you try initializing a model with degrees of freedom, then how do you know that the answer will actually be meaningful?
If this check is failing, it indicates that something is going on that you really need to investigate.
You should not be checking this after initialization as part of the routine - you definitely should have a test that checks for that however.
Finally, I am not sure on WaterTAPs plans for this, but note that IDAES has a new initialization API that handles a lot of this for you and that we do plan to eventually deprecate the current API.
There was a problem hiding this comment.
@luohezhiming I'm not sure why the initialization routine in modified_ADM1_flowsheet wasn't working as intended, but I've updated it to use SequentialDecomposition, and now the DOF check works fine.
ksbeattie
added
Priority:High
There was a problem hiding this comment.
LGTM - just one note for whoever the next reviewer is. This PR doesn't fix the underlying issue with the costing function (I'll continue to explore this issue in #1468 once this is merged), but it does allow the system to reach the "real" optimal solution whereas the previous solution was only sub-optimal.
Feel free to correct me if I've misunderstood something @luohezhiming
| m.fs.FeedWater.temperature.fix(308.15 * pyo.units.K) | ||
| m.fs.FeedWater.pressure.fix(1 * pyo.units.atm) | ||
|
|
||
| if bio_P is True: |
There was a problem hiding this comment.
From an extremely quick glance, it looks like you are using two different feed compositions depending on whether bio_P is True or False. Other than helping with model convergence, I am not sure why we would do this. In my mind, we would want to be able to observe the difference in results, while using the same feed composition, between applying bio_P=True vs. bio_P=False.
There was a problem hiding this comment.
The two sources feed composition are corresponding to the the simulation results from BMS2_P_extension.py, with BioP is False or True, we get different AD inlet concetrations. I want to test the stablility of the flowsheet with the background of BSM2
| m.fs.props_ASM2D = ModifiedASM2dParameterBlock() | ||
| m.fs.rxn_props_ASM2D = ModifiedASM2dReactionParameterBlock( | ||
| property_package=m.fs.props_ASM2D |
There was a problem hiding this comment.
Technically, this modified_ADM1_flowsheet should only comprise ADM1 and AD unit, not ASM2D and associated translator blocks. Ideally this would be a separate example, e.g., anerobic_digestion_with_ASM2D_translation. My main point is that this flowsheet is more than just modified_ADM1_flowsheet.
There was a problem hiding this comment.
OK. I will modified this flowsheet as modified_ADM1_flowsheet_with_translators.py
| (0, "S_A"): 0.10, | ||
| (0, "S_F"): 0.16, | ||
| (0, "S_I"): 0.05745, | ||
| (0, "S_N2"): 0.025, | ||
| (0, "S_NH4"): 0.04, | ||
| (0, "S_NO3"): 1e-9, | ||
| (0, "S_O2"): 0.0014, | ||
| (0, "S_PO4"): 0.026, | ||
| (0, "S_I"): 0.057, | ||
| (0, "S_N2"): 0.036, | ||
| (0, "S_NH4"): 0.03, | ||
| (0, "S_NO3"): 0.002, | ||
| (0, "S_O2"): 0.0013, | ||
| (0, "S_PO4"): 0.024, | ||
| (0, "S_K"): 0.38, | ||
| (0, "S_Mg"): 0.028, | ||
| (0, "S_IC"): 0.075, | ||
| (0, "X_AUT"): 1e-9, | ||
| (0, "X_H"): 22.0, | ||
| (0, "X_I"): 10.8, | ||
| (0, "X_PAO"): 11.8, | ||
| (0, "X_PHA"): 0.0072, | ||
| (0, "X_PP"): 3.17, | ||
| (0, "X_S"): 3.71, | ||
| (0, "S_Mg"): 0.027, | ||
| (0, "S_IC"): 0.072, | ||
| (0, "X_AUT"): 0.25, | ||
| (0, "X_H"): 23.0, | ||
| (0, "X_I"): 11.3, | ||
| (0, "X_PAO"): 10.8, | ||
| (0, "X_PHA"): 0.0058, | ||
| (0, "X_PP"): 2.9, | ||
| (0, "X_S"): 3.8, |
There was a problem hiding this comment.
Wondering if you came up with these guesses in a formulaic way, in relation to the feed composition, or if you came up with these purely through trial and error. Preferably, the tear guesses would have some known relationship with the feed composition so that we wouldn't need to manually adjust these when changing the feed.
There was a problem hiding this comment.
The tear guesses are supposed to be initial value we guessed for the place we cut the loop, we don't know the exact values and it cannot be directly derive from the feed composition. I get these guessed values from multiple attempts of simulations, it is relevant stable with feed composition, though, if we change the feed, the flowsheet can still be solved with the tear_guess I set.
| 4.453e-06, rel=1e-3 | ||
| 6.4300e-07, rel=1e-3 |
There was a problem hiding this comment.
I this meant to be a ~zero value? Wondering if this should be changed to absolute tolerance or not.
There was a problem hiding this comment.
Either way, for values this small you need to be using absolute tolerances (unless the model is really well scaled), as otherwise they are below solver tolerance and thus only accurate to maybe 1e-6 (absolute).
There was a problem hiding this comment.
@luohezhiming can you change this to absolute tolerance before I merge?
| ) == pytest.approx(0, abs=1e-6) | ||
| ) == pytest.approx(0.00041597, rel=1e-3) |
There was a problem hiding this comment.
Seems questionable that a non-negligible amount of X_AUT would end up in the treated effluent when it was previously ~0. I wonder if any existing results from the literature would help us verify whether a non-negligible amount of X_AUT in the treated effluent is a reasonable expectation here.
There was a problem hiding this comment.
From what I test, the previous zero result is unstable and tests cannot pass on github even though it is an optimal solution local. And I believe the current result is correct because if you check the output of ASM2d_ADM1_translator, the zero_flow_components are all reset to 1e-10 as we coded in the translator block, the previous results are not
| assert value(m.fs.Treated.properties[0].conc_mass_comp["X_H"]) == pytest.approx( | ||
| 0.0125387, rel=1e-3 | ||
| 0.013371, rel=1e-3 | ||
| ) | ||
| assert value(m.fs.Treated.properties[0].conc_mass_comp["X_I"]) == pytest.approx( | ||
| 0.0119245, rel=1e-3 | ||
| 0.012360, rel=1e-3 | ||
| ) | ||
| assert value( | ||
| m.fs.Treated.properties[0].conc_mass_comp["X_PAO"] | ||
| ) == pytest.approx(0.0132445, rel=1e-3) | ||
| ) == pytest.approx(0.011077, rel=1e-3) | ||
| assert value( | ||
| m.fs.Treated.properties[0].conc_mass_comp["X_PHA"] | ||
| ) == pytest.approx(6.445095e-06, rel=1e-3) | ||
| ) == pytest.approx(5.0389e-06, rel=1e-3) | ||
| assert value( | ||
| m.fs.Treated.properties[0].conc_mass_comp["X_PP"] | ||
| ) == pytest.approx(0.00441488, rel=1e-3) | ||
| ) == pytest.approx(0.0036998, rel=1e-3) | ||
| assert value(m.fs.Treated.properties[0].conc_mass_comp["X_S"]) == pytest.approx( | ||
| 0.00021346, rel=1e-3 | ||
| 0.00021570, rel=1e-3 |
There was a problem hiding this comment.
I suppose my uncertainty has to do with the concentrations of X components that we should expect to see in treated effluent (at least in terms of order of magnitude).
| iscale.set_scaling_factor( | ||
| m.fs.unit.liquid_phase.properties_out[0].conc_mass_comp["S_IP"], 1e-5 | ||
| ) | ||
| iscale.set_scaling_factor( | ||
| m.fs.unit.liquid_phase.properties_out[0].conc_mass_comp["S_IN"], 1e-5 | ||
| ) |
There was a problem hiding this comment.
I thought that you wanted to remove these. My guess is that the model will not converge without these factors. Is that right? Another thing to consider here, if that is the case, is whether the scaled variable values end up being small enough to satisfy constraint tolerances, leading to erroneous convergence.
There was a problem hiding this comment.
The model will not converge with this settings in the tests, I temporarily reset the scaling factor of these two components to 1e-5 to pass the tests. If we change the inputs, the model can converge.
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:
Modified ADM1 flowsheet
Changes proposed in this PR:
Legal Acknowledgement
By contributing to this software project, I agree to the following terms and conditions for my contribution: