-
Notifications
You must be signed in to change notification settings - Fork 62
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
Anaerobic Digester Re-Scaling #1535
base: main
Are you sure you want to change the base?
Changes from 37 commits
7531440
059803e
a0921cd
367f18c
40ab286
03e970a
863e6b3
01427e9
2bcc1b6
9b624d9
07aabdd
7b73282
ff3a127
7a6867b
f6203ea
de4ca55
2e9aaaf
8943b4e
9797a52
c7f05a6
3313ef2
9eb6984
1e57393
f75f4b0
0dc4b4f
5dda788
07688ab
0fa813e
01ffa20
1086276
93862e7
5f467b5
ac27d2b
b83aded
e7758fc
a05549a
3a54930
448b18a
aa789c0
10c9583
7c8becb
7681d4b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -940,6 +940,9 @@ def model(self): | |
m.fs.unit.liquid_phase.properties_in[0].TSS | ||
m.fs.unit.liquid_phase.properties_out[0].TSS | ||
|
||
iscale.calculate_scaling_factors(m) | ||
iscale.set_scaling_factor(m.fs.unit.liquid_phase.heat, 1e-6) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. WHy did you need this change? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This change is not needed. It may have been necessary at some point while I was working on this PR, but I'll undo it |
||
|
||
return m | ||
|
||
@pytest.mark.unit | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -67,18 +67,125 @@ | |
from idaes.core.util.constants import Constants | ||
from idaes.core.util.exceptions import ConfigurationError, InitializationError | ||
from idaes.core.util.tables import create_stream_table_dataframe | ||
from idaes.core.scaling import CustomScalerBase, ConstraintScalingScheme | ||
|
||
from watertap.costing.unit_models.anaerobic_digester import cost_anaerobic_digester | ||
|
||
__author__ = "Alejandro Garciadiego, Andrew Lee, Xinhong Liu" | ||
|
||
|
||
class ADScaler(CustomScalerBase): | ||
""" | ||
Default modular scaler for anaerobic digester. | ||
|
||
This Scaler relies on the associated property and reaction packages, | ||
either through user provided options (submodel_scalers argument) or by default | ||
Scalers assigned to the packages. | ||
""" | ||
|
||
DEFAULT_SCALING_FACTORS = { | ||
"volume": 1e-2, | ||
} | ||
|
||
def variable_scaling_routine( | ||
self, model, overwrite: bool = False, submodel_scalers: dict = None | ||
): | ||
""" | ||
Routine to apply scaling factors to variables in model. | ||
|
||
Args: | ||
model: model to be scaled | ||
overwrite: whether to overwrite existing scaling factors | ||
submodel_scalers: dict of Scalers to use for sub-models, keyed by submodel local name | ||
|
||
Returns: | ||
None | ||
""" | ||
# Call scaling methods for sub-models | ||
self.call_submodel_scaler_method( | ||
submodel=model.liquid_phase.properties_in, | ||
method="variable_scaling_routine", | ||
submodel_scalers=submodel_scalers, | ||
overwrite=overwrite, | ||
) | ||
self.propagate_state_scaling( | ||
target_state=model.liquid_phase.properties_out, | ||
source_state=model.liquid_phase.properties_in, | ||
overwrite=overwrite, | ||
) | ||
|
||
self.call_submodel_scaler_method( | ||
submodel=model.liquid_phase.properties_out, | ||
method="variable_scaling_routine", | ||
submodel_scalers=submodel_scalers, | ||
overwrite=overwrite, | ||
) | ||
self.call_submodel_scaler_method( | ||
submodel=model.liquid_phase.reactions, | ||
method="variable_scaling_routine", | ||
submodel_scalers=submodel_scalers, | ||
overwrite=overwrite, | ||
) | ||
|
||
# Scaling control volume variables | ||
self.scale_variable_by_default( | ||
model.liquid_phase.volume[0], overwrite=overwrite | ||
) | ||
|
||
def constraint_scaling_routine( | ||
self, model, overwrite: bool = False, submodel_scalers: dict = None | ||
): | ||
""" | ||
Routine to apply scaling factors to constraints in model. | ||
|
||
Submodel Scalers are called for the property and reaction blocks. All other constraints | ||
are scaled using the inverse maximum scheme. | ||
|
||
Args: | ||
model: model to be scaled | ||
overwrite: whether to overwrite existing scaling factors | ||
submodel_scalers: dict of Scalers to use for sub-models, keyed by submodel local name | ||
|
||
Returns: | ||
None | ||
""" | ||
# Call scaling methods for sub-models | ||
self.call_submodel_scaler_method( | ||
submodel=model.liquid_phase.properties_in, | ||
method="constraint_scaling_routine", | ||
submodel_scalers=submodel_scalers, | ||
overwrite=overwrite, | ||
) | ||
self.call_submodel_scaler_method( | ||
submodel=model.liquid_phase.properties_out, | ||
method="constraint_scaling_routine", | ||
submodel_scalers=submodel_scalers, | ||
overwrite=overwrite, | ||
) | ||
self.call_submodel_scaler_method( | ||
submodel=model.liquid_phase.reactions, | ||
method="constraint_scaling_routine", | ||
submodel_scalers=submodel_scalers, | ||
overwrite=overwrite, | ||
) | ||
|
||
# Scale unit level constraints | ||
if hasattr(model, "AD_retention_time"): | ||
self.scale_constraint_by_nominal_value( | ||
model.AD_retention_time[0], | ||
scheme=ConstraintScalingScheme.inverseMaximum, | ||
overwrite=overwrite, | ||
) | ||
|
||
|
||
@declare_process_block_class("AD") | ||
class ADData(UnitModelBlockData): | ||
""" | ||
AD Unit Model Class | ||
""" | ||
|
||
default_scaler = ADScaler | ||
|
||
CONFIG = UnitModelBlockData.CONFIG() | ||
|
||
CONFIG.declare( | ||
|
@@ -366,7 +473,7 @@ def build(self): | |
) | ||
|
||
# --------------------------------------------------------------------- | ||
# Check flow basis is compatable | ||
# Check flow basis is compatible | ||
# TODO : Could add code to convert flow bases, but not now | ||
t_init = self.flowsheet().time.first() | ||
if ( | ||
|
@@ -792,22 +899,6 @@ def rule_electricity_consumption(self, t): | |
# Set references to balance terms at unit level | ||
self.heat_duty = Reference(self.liquid_phase.heat[:]) | ||
|
||
iscale.set_scaling_factor(self.KH_co2, 1e2) | ||
iscale.set_scaling_factor(self.KH_ch4, 1e2) | ||
iscale.set_scaling_factor(self.KH_h2, 1e2) | ||
iscale.set_scaling_factor(self.hydraulic_retention_time, 1e-6) | ||
iscale.set_scaling_factor(self.volume_AD, 1e-2) | ||
iscale.set_scaling_factor(self.volume_vapor, 1e-2) | ||
iscale.set_scaling_factor(self.liquid_phase.rate_reaction_generation, 1e4) | ||
iscale.set_scaling_factor(self.liquid_phase.mass_transfer_term, 1e2) | ||
iscale.set_scaling_factor(self.liquid_phase.heat, 1e0) | ||
iscale.set_scaling_factor(self.liquid_phase.rate_reaction_extent, 1e4) | ||
iscale.set_scaling_factor(self.liquid_phase.enthalpy_transfer, 1e0) | ||
iscale.set_scaling_factor(self.liquid_phase.volume, 1e-2) | ||
iscale.set_scaling_factor(self.electricity_consumption, 1e0) | ||
for i, c in self.ad_performance_eqn.items(): | ||
iscale.constraint_scaling_transform(c, 1e2) | ||
|
||
def _get_stream_table_contents(self, time_point=0): | ||
return create_stream_table_dataframe( | ||
{ | ||
|
@@ -836,6 +927,21 @@ def calculate_scaling_factors(self): | |
& self.liquid_phase.properties_out.component_list | ||
) | ||
|
||
iscale.set_scaling_factor(self.KH_co2, 1e2) | ||
iscale.set_scaling_factor(self.KH_ch4, 1e2) | ||
iscale.set_scaling_factor(self.KH_h2, 1e2) | ||
iscale.set_scaling_factor(self.hydraulic_retention_time, 1e-6) | ||
iscale.set_scaling_factor(self.volume_AD, 1e-2) | ||
iscale.set_scaling_factor(self.volume_vapor, 1e-2) | ||
iscale.set_scaling_factor(self.liquid_phase.rate_reaction_generation, 1e4) | ||
iscale.set_scaling_factor(self.liquid_phase.mass_transfer_term, 1e2) | ||
iscale.set_scaling_factor(self.liquid_phase.rate_reaction_extent, 1e4) | ||
iscale.set_scaling_factor(self.liquid_phase.enthalpy_transfer, 1e0) | ||
iscale.set_scaling_factor(self.liquid_phase.volume, 1e-2) | ||
iscale.set_scaling_factor(self.electricity_consumption, 1e0) | ||
Comment on lines
+930
to
+941
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Shouldn't these be handled in the scaler class? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Andrew mentioned that we should not mix and match the new and old scaling tools. In other words, the scaler class should not make use of iscale. This is the reason why I've moved all of the If your question is why don't I set these default scaling factors in the ADScaler class, it's because I figured these would make more sense as user-defined scaling factors. I know I did do some testing with having default scaling factors for these variables in the new scaler class, but I forget how thoroughly I played around with this. Currently, the only variable scaled by default in the new scaler class is volume. |
||
for i, c in self.ad_performance_eqn.items(): | ||
iscale.constraint_scaling_transform(c, 1e2) | ||
|
||
# TODO: improve this later; for now, this resolved some scaling issues for modified adm1 test file | ||
if "S_IP" in self.config.liquid_property_package.component_list: | ||
sf = iscale.get_scaling_factor( | ||
|
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.
Would it make sense to check the condition number at the flowsheet level between old scaling approach and new?
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.
Then, every time another PR impacts scaling at this flowsheet level, we can track whether condition number improves or not as we go.