-
Notifications
You must be signed in to change notification settings - Fork 58
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
BSM2-P Effluent Metrics w/o Flowsheet Constraints #1492
Conversation
# for v in metadata.list_supported_properties(): | ||
# if metadata[v.name].method is not None: | ||
# if model.props[1].is_property_constructed(v.name): | ||
# raise PropertyAttributeError( | ||
# "Property {v_name} is an on-demand property, but was found " | ||
# "on the stateblock without being demanded".format(v_name=v.name) | ||
# ) |
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.
Why comment these out?
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 think this test was written to check that the constraints for VSS, ISS, TSS, COD, etc. were only built on-demand, but I've changed them from constraints to expressions, so I don't think this test applies anymore. On that note, I don't think I need to be touching the effluent properties in the flowsheet anymore either.
) | ||
print( | ||
"BOD5 concentration", | ||
pyo.value(m.fs.FeedWater.properties[0].BOD5["effluent"]), |
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.
Should the index be influent instead of effluent?
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.
Yes - although technically it should be "raw". I honestly forgot I even printed the influent metrics already, but this is just something I added without putting too much thought in. I should verify that the equations for influent really are identical to the equations for effluents. We know, at the very least, that the equation for BOD5 differs slightly, which is why we have this config option implemented
iscale.set_scaling_factor(self.TSS, 1e1) | ||
|
||
if self.is_property_constructed("COD"): | ||
if iscale.get_scaling_factor(self.COD) is None: | ||
iscale.set_scaling_factor(self.COD, 1e1) | ||
|
||
if self.is_property_constructed("BOD5"): | ||
if iscale.get_scaling_factor(self.BOD5) is None: | ||
iscale.set_scaling_factor(self.BOD5, 1e1) | ||
|
||
if self.is_property_constructed("SNKj"): | ||
if iscale.get_scaling_factor(self.SNKj) is None: | ||
iscale.set_scaling_factor(self.SNKj, 1e2) | ||
|
||
if self.is_property_constructed("SNOX"): | ||
if iscale.get_scaling_factor(self.SNOX) is None: | ||
iscale.set_scaling_factor(self.SNOX, 1e3) | ||
|
||
if self.is_property_constructed("SP_organic"): | ||
if iscale.get_scaling_factor(self.SP_organic) is None: | ||
iscale.set_scaling_factor(self.SP_organic, 1e2) | ||
|
||
if self.is_property_constructed("SP_inorganic"): | ||
if iscale.get_scaling_factor(self.SP_inorganic) is None: | ||
iscale.set_scaling_factor(self.SP_inorganic, 1e3) |
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.
Do we need to scale the expressions? Have you tried without this added scaling?
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.
Just tried removing scaling in the latest commit - it still fails
"Total suspended solids concentration", ":math:`TSS_{out} <= TSS_{max}`" | ||
"Chemical oxygen demand", ":math:`COD_{out} <= COD_{max}`" | ||
"Total phosphorus concentration", ":math:`P_{out} <= P_{max}`" | ||
"Total nitrogen concentration", ":math:`N_{out} <= N_{max}`" | ||
"5-day biological oxygen demand", ":math:`BOD5_{out} <= BOD5_{max}`" |
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.
Should this be removed until we are able to incorporate as constraints?
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.
Sure - I'll get rid of the change in this PR and leave it in the follow-up PR (1503)
# check that properties are not built if not demanded | ||
for v in metadata.list_supported_properties(): | ||
if metadata[v.name].method is not None: | ||
if model.props[1].is_property_constructed(v.name): | ||
raise PropertyAttributeError( | ||
"Property {v_name} is an on-demand property, but was found " | ||
"on the stateblock without being demanded".format(v_name=v.name) | ||
) | ||
|
||
# check that properties are built if demanded | ||
for v in metadata.list_supported_properties(): | ||
if metadata[v.name].method is not None: | ||
if not hasattr(model.props[1], v.name): | ||
raise PropertyAttributeError( | ||
"Property {v_name} is an on-demand property, but was not built " | ||
"when demanded".format(v_name=v.name) |
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.
can you remind me why this has to be removed?
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.
Theses tests were originally written to check that the constraints for VSS, ISS, and TSS (which were present before this PR) were only built on-demand, but I've changed them from constraints to expressions, so I think these tests don't apply anymore. If we were to keep them, the test that checks that properties are not built if not demanded would fail
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.
Nice work! I am curious how scaling became an issue after including expressions. I wonder if it had something to do with taking away variables/constraints, which had scaling, and replacing with the expressions. In effect, model scaling was slightly altered to a less stable form.
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.
Some minor change needs to be addressed and a few comments
|
||
self.eq_ISS = pyo.Constraint(rule=rule_ISS) | ||
self.SNKj = pyo.Expression(rule=_SNKj, doc="Kjeldahl nitrogen") |
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 am curious why SNKj is short for "Kjeldahl nitrogen", I remember generally we called "total Kjeldahl nitrogen" as TKN
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.
The Flores-Alsina files are inconsistent when naming this variable. Sometimes they simply refer to it as Kjeldahl nitrogen and other times they refer to it as total Kjeldahl nitrogen. I opted to use the former, but I agree with you that we should just refer to this as TKN, even though Flores-Alsina always denotes it as SNKj
) | ||
print( | ||
"SNOX concentration", | ||
pyo.value(m.fs.Treated.properties[0].SNOX), |
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.
pyo.value(m.fs.Treated.properties[0].SNOX), | |
pyo.value(m.fs.FeedWater.properties[0].SNOX), |
print( | ||
"SNOX concentration", | ||
pyo.value(m.fs.Treated.properties[0].SNOX), | ||
pyo.units.get_units(m.fs.Treated.properties[0].SNOX), |
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.
pyo.units.get_units(m.fs.Treated.properties[0].SNOX), | |
pyo.units.get_units(m.fs.FeedWater.properties[0].SNOX), |
Fixes/Resolves:
This PR is similar to #1489, but it does not attempt to implement the effluent violation constraints in the BSM2_P_extension flowsheet - this will be handled in a separate PR. The constraints related to the effluent metrics are derived from the work of Flores-Alsina (specifically perf_plant_ss.m).
Summary/Motivation:
Define the effluent metrics for the BSM2_P_extension 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: