New pressure changer and stoichiometric scalers #1678
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## scaling_toolbox #1678 +/- ##
===================================================
+ Coverage 77.11% 77.15% +0.03%
===================================================
Files 395 395
Lines 63727 63765 +38
Branches 10555 10567 +12
===================================================
+ Hits 49145 49195 +50
+ Misses 12079 12077 -2
+ Partials 2503 2493 -10 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| # for the isentropic assumption, a duplicate properties block is created from the unit model's | ||
| # property package and can be scaled by calling the properties scaler | ||
| if hasattr(model, "properties_isentropic"): | ||
| self.call_submodel_scaler_method( | ||
| submodel=model.properties_isentropic, | ||
| method="variable_scaling_routine", | ||
| submodel_scalers=submodel_scalers, | ||
| overwrite=overwrite, | ||
| ) |
There was a problem hiding this comment.
You should propagate scaling factors from control_volume.properties_in to properties_isentropic.
| if hasattr(model, "work_fluid"): | ||
| for t in model.flowsheet().time: | ||
| self.scale_variable_by_component( | ||
| model.work_fluid[t], model.control_volume.work[t] | ||
| ) | ||
|
|
||
| if hasattr(model, "work_isentropic"): | ||
| for t in model.flowsheet().time: | ||
| self.scale_variable_by_component( | ||
| model.work_isentropic[t], model.control_volume.work[t] | ||
| ) | ||
|
|
||
| if hasattr(model, "work_mechanical"): | ||
| for t in model.flowsheet().time: | ||
| self.scale_variable_by_component( | ||
| model.work_mechanical[t], model.control_volume.work[t] | ||
| ) |
There was a problem hiding this comment.
Are all of these Vars, or is one of them a Reference to control_volume.work?
There was a problem hiding this comment.
work_mechanical is a reference and was removed. The other two are Vars
| for c in model.component_data_objects(Constraint, descend_into=False): | ||
| self.scale_constraint_by_nominal_value( | ||
| c, | ||
| overwrite=overwrite, | ||
| ) | ||
|
|
||
|
|
There was a problem hiding this comment.
Are you sure that this constraint scaling scheme is appropriate for every constraint in the model?
There was a problem hiding this comment.
I tested this a couple ways. For a handful of example cases that include all of the thermodynamic assumptions, I tested each scheme applied to every constraint and inverseMaximum worked the best. The diagnostics toolbox also doesn't report any extreme constraints when using inverseMaximum .
I have a version that lists each constraint instead of this method of applying to every constraint that I can use instead. That way the scheme can be changed for a specific constraint if any problem arises in the future.
| assert StoichiometricReactor().default_scaler == StoichiometricReactorScaler | ||
|
|
||
| @pytest.mark.integration | ||
| def test_example_case(self): |
There was a problem hiding this comment.
Could you please split this into two tests? One should be a component test that checks the scaling factors, the other should be an integration test that performs the solve. You can use a pytest fixture with a "class" scope in order to use the same concrete model object throughout the test.
There was a problem hiding this comment.
Added this in new commits. The StoichiometricReactor unit model doesn't have any of its own variables or constraints to test, so I descended into control_volume , properties_in, and properties_out to test scaling factors.
Summary/Motivation:
Redoing the PR for creating scalers for the pressure changer and stoichiometric reactor unit models.
Legal Acknowledgement
By contributing to this software project, I agree to the following terms and conditions for my contribution: