-
Notifications
You must be signed in to change notification settings - Fork 74
CSTR Re-Scaling #1542
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
Merged
Merged
CSTR Re-Scaling #1542
Changes from all commits
Commits
Show all changes
22 commits
Select commit
Hold shift + click to select a range
88cca29
Update test files
MarcusHolly c0ec701
Merge branch 'main' into cstr_scaling
MarcusHolly cd5c8fa
Address pylint issue
MarcusHolly 69dde0f
Create aeratoin tank scaler and tests
MarcusHolly 3387020
Address pylint issues
MarcusHolly 3e8836d
Add initial Scaler Profiler testing
MarcusHolly 755456c
Skip ScalingProfiler tests on Mac
MarcusHolly 14d8f9b
Perturb injection for ScalingProfiler tests
MarcusHolly 938f6e3
Merge branch 'main' into cstr_scaling
MarcusHolly 46b1205
Modify scaling routines
MarcusHolly 3b3b511
Merge branch 'main' into cstr_scaling
MarcusHolly 85f3665
Add scaling factor for injection to cstr_injection
MarcusHolly 32f76f1
Add injection scaling to aeration tank
MarcusHolly 64dd1a2
Remove redundant constraint scaling
MarcusHolly 67b0449
Adjust constraint scaling
MarcusHolly 2fcf9b5
Merge branch 'main' into cstr_scaling
MarcusHolly bc479e4
Trim tests by removing asserts for individual scaling factors
MarcusHolly f681f11
Remove unnecessary TODO note
MarcusHolly c78aaed
Merge branch 'main' into cstr_scaling
MarcusHolly 0182600
Add additional checks for condition numbers
MarcusHolly 494763d
Merge branch 'main' into cstr_scaling
ksbeattie 5a520f6
Merge branch 'main' into cstr_scaling
adam-a-a File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -15,11 +15,13 @@ | |
|
||
# Import Pyomo libraries | ||
from pyomo.common.config import In | ||
from pyomo.environ import Constraint | ||
|
||
# Import IDAES cores | ||
from idaes.core import ( | ||
declare_process_block_class, | ||
) | ||
from idaes.core.scaling import CustomScalerBase, ConstraintScalingScheme | ||
|
||
|
||
from watertap.unit_models.cstr_injection import ( | ||
|
@@ -30,12 +32,147 @@ | |
__author__ = "Adam Atia" | ||
|
||
|
||
class AerationTankScaler(CustomScalerBase): | ||
""" | ||
Default modular scaler for the aeration tank unit model. | ||
|
||
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-3, | ||
"hydraulic_retention_time": 1e-3, | ||
"KLa": 1e-1, | ||
Comment on lines
+45
to
+47
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. Let's document these default scaling factors an issue in case they ever become suspects in a "failure to converge" crime scene. I won't hold this up anymore! |
||
"mass_transfer_term": 1e2, | ||
} | ||
|
||
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.control_volume.properties_in, | ||
method="variable_scaling_routine", | ||
submodel_scalers=submodel_scalers, | ||
overwrite=overwrite, | ||
) | ||
self.propagate_state_scaling( | ||
target_state=model.control_volume.properties_out, | ||
source_state=model.control_volume.properties_in, | ||
overwrite=overwrite, | ||
) | ||
|
||
self.call_submodel_scaler_method( | ||
submodel=model.control_volume.properties_out, | ||
method="variable_scaling_routine", | ||
submodel_scalers=submodel_scalers, | ||
overwrite=overwrite, | ||
) | ||
self.call_submodel_scaler_method( | ||
submodel=model.control_volume.reactions, | ||
method="variable_scaling_routine", | ||
submodel_scalers=submodel_scalers, | ||
overwrite=overwrite, | ||
) | ||
|
||
# Scaling control volume variables | ||
self.scale_variable_by_default( | ||
model.control_volume.volume[0], overwrite=overwrite | ||
) | ||
self.scale_variable_by_default( | ||
model.hydraulic_retention_time[0], overwrite=overwrite | ||
) | ||
self.scale_variable_by_default(model.KLa, overwrite=overwrite) | ||
if model.config.has_aeration: | ||
if "S_O" and "S_O2" in model.config.property_package.component_list: | ||
self.scale_variable_by_default( | ||
model.control_volume.mass_transfer_term[0, "Liq", "S_O"], | ||
overwrite=overwrite, | ||
) | ||
self.scale_variable_by_default( | ||
model.control_volume.mass_transfer_term[0, "Liq", "S_O2"], | ||
overwrite=overwrite, | ||
) | ||
elif "S_O" in model.config.property_package.component_list: | ||
self.scale_variable_by_default( | ||
model.control_volume.mass_transfer_term[0, "Liq", "S_O"], | ||
overwrite=overwrite, | ||
) | ||
elif "S_O2" in model.config.property_package.component_list: | ||
self.scale_variable_by_default( | ||
model.control_volume.mass_transfer_term[0, "Liq", "S_O2"], | ||
overwrite=overwrite, | ||
) | ||
else: | ||
pass | ||
|
||
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.control_volume.properties_in, | ||
method="constraint_scaling_routine", | ||
submodel_scalers=submodel_scalers, | ||
overwrite=overwrite, | ||
) | ||
self.call_submodel_scaler_method( | ||
submodel=model.control_volume.properties_out, | ||
method="constraint_scaling_routine", | ||
submodel_scalers=submodel_scalers, | ||
overwrite=overwrite, | ||
) | ||
self.call_submodel_scaler_method( | ||
submodel=model.control_volume.reactions, | ||
method="constraint_scaling_routine", | ||
submodel_scalers=submodel_scalers, | ||
overwrite=overwrite, | ||
) | ||
|
||
# Scale unit level constraints | ||
for c in model.component_data_objects(Constraint, descend_into=True): | ||
self.scale_constraint_by_nominal_value( | ||
c, | ||
scheme=ConstraintScalingScheme.inverseMaximum, | ||
overwrite=overwrite, | ||
) | ||
|
||
|
||
@declare_process_block_class("AerationTank") | ||
class AerationTankData(CSTR_InjectionData): | ||
""" | ||
CSTR Unit Model with Injection Class | ||
""" | ||
|
||
default_scaler = AerationTankScaler | ||
|
||
CONFIG = CSTR_InjectionData.CONFIG() | ||
CONFIG.electricity_consumption = ElectricityConsumption.calculated | ||
CONFIG.get("has_aeration")._domain = In([True]) | ||
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
any reason why we wouldn't use the inverse of the variable value (assuming it was fixed)?
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.
So basically why don't we use the autoscaling functions for these variables? In general, the autoscaling tools haven't been working great for me when applying them in a wide-sweeping sense like this. They're also just used not used very much in general. I haven't seen any examples of the new Scaler Objects using the autoscaling tools
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.
Fixed variables are not visible to IPOPT at all. The only reason you might need a scaling factor for them is if some other quantity depended on it.
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.
That's true...forgot about that. What I really meant was "assuming the inverse of the variable value would be tolerable".
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 don't think we're aiming for "perfect scaling". I'm rusty on the mathematical reasons why we'd want to avoid it, but I know it was discussed in one of Andrew's scaling documents. Does anybody happen to have/know where that up-to-date scaling document is?
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.
Unsure, but my only concern is that volume would be tied to define flowrate, which can be whatever the user wants to specify. So for a very large system, volumes would be larger, and the scaling factor might not be small enough (for example).
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.
Additionally, volume divided by volumetric flowrate equals HRT, so it would make more sense to set a scaling factor for volume that is based on the scaling factors for HRT and volumetric flowrate (for example).
Actually, since volumetric flowrate is a state variable in ASM/ADM models, the user should always provide scaling for that. Now, I think we can safely assume some default for HRT, based on typical HRT values. From that, we can compute the scaling factor for volume as scaling_factor_of_HRT * scaling_factor_of_volumetric_flowrate
Uh oh!
There was an error while loading. Please reload this page.
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.
Thsoe are valid concerns... the user would just have to update these scaling factors manually for a large system. To give some background on why I chose these values, 1e-3 was a good scaling factor for HRT and volume for the CSTRs in the BSM models. This implies that the magnitude of the flow would be on the scale of 1 (equivalent to no scaling factor), which checks out after looking at 2 CSTRs in the BSM2 flowsheet. For the sake of transparency, I could add a scaling factor of 1 to the list of default scaling factors for volumetric flow.
TLDR: I'm not opposed to what you're suggesting, but I'm not sure how I would implement it, especially given Doug's comment
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.
What Andrew meant by "perfect scaling" was scaling every variable using a scaling factor of
1/abs(value(var))
. We don't want to do that because it typically screws up the Jacobian scaling. The best example is something like enthalpy, where the variable value can pass through zero. If you scale based on an initial condition for which the enthalpy just happens to be a number close to zero, that causes major problems.Variables that pass through zero aren't the only case where naive scaling like that causes problems, but they're the most obvious.