BSM2-P Effluent Metrics #1489
BSM2-P Effluent Metrics #1489
Conversation
MarcusHolly
requested review from
luohezhiming,
adam-a-a and
agarciadiego
and removed request for
luohezhiming and
adam-a-a
watertap/property_models/unit_specific/activated_sludge/modified_asm2d_properties.py
Outdated
Show resolved
Hide resolved
| @@ -210,6 +210,86 @@ def build(self): | |||
| doc="ISS fractional content of biomass", | |||
| ) | |||
|
|
|||
| # EQI parameters | |||
There was a problem hiding this comment.
Can you add some general refs so we know where to look for these in future?
| self.eq_SNKj = pyo.Constraint(rule=rule_SNKj) | ||
|
|
||
| def rule_BOD5(b): | ||
| return b.BOD5 == 0.25 * ( |
There was a problem hiding this comment.
From memory, I recall that the factor (0.25) could change depending on whether this is calculated for influent or effluent. Can you double-check this? I handled this in some way for ASM1 (though it might not be ideal approach).
There was a problem hiding this comment.
If I am recalling correctly, I would at least opt for using a mutable Param instead of the hard-coded 0.25. Then it would be on the user to change the Param value as needed at the flowsheet level.
There was a problem hiding this comment.
I am familiar with what you are talking about, but the c-code and the literature reference both hard-code 0.25, so I didn't see a need to do what you did in ASM1
There was a problem hiding this comment.
The problem here is if I want to apply to the influent, that value changes, right? Since this can be constructed on any state block and not only one that corresponds to treated effluent, we cannot just hard-code the value to what would correspond to the effluent.
There was a problem hiding this comment.
Can you show the same snapshot for BOD5i or whatever corresponds to influent in that file?
There was a problem hiding this comment.
Ah yes, the influent value is 0.65
There was a problem hiding this comment.
My initial response to this PR before dropping any suggestions was "WOW--that was fast!"
Great work--I think some additional ASM2d testing on the new metrics, just as a sanity check, would be good. Most of all though, fingers crossed that the flowsheet can still solve.
| m.fs.Treated.properties[0].TSS | ||
| m.fs.Treated.properties[0].COD | ||
| m.fs.Treated.properties[0].BOD5 | ||
| m.fs.Treated.properties[0].SNKj | ||
| m.fs.Treated.properties[0].SP_organic | ||
| m.fs.Treated.properties[0].SP_inorganic |
There was a problem hiding this comment.
You should touch these before the solve.
There was a problem hiding this comment.
These are touched before the solve. I placed it at the end of set_operating_conditions. Do you mean these should be touched right before the solve? How would that functionally be any different than the current placement?
There was a problem hiding this comment.
My mistake- I misread the GH diff and it looked as if these lines were after the solve. That said, I wonder if it makes more sense to touch before applying scaling. Right now, I think you made each an expression so it might not matter. If they were reverted back to vars, then it would matter.
| def add_effluent_violations(m): | ||
| # TODO: update "m" to blk; change ref to m.fs.Treated instead of CL1 effluent |
There was a problem hiding this comment.
I haven't been able to figure out why, but this change is breaking the BSM2 GUI, so I've reverted it in #1491
|
Replaced by #1503 |
Summary/Motivation:
Adds effluent concentration violations to the BSM2-P 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: