Scaler for equilibrium reactor and saponification properties#1500
Scaler for equilibrium reactor and saponification properties#1500andrewlee94 merged 84 commits intoIDAES:mainfrom
Conversation
andrewlee94
requested review from
bpaul4,
dallan-keylogic,
dangunter and
lbianchi-lbl
as code owners
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1500 +/- ##
=======================================
Coverage 76.98% 76.99%
=======================================
Files 382 382
Lines 61911 61993 +82
Branches 10126 10146 +20
=======================================
+ Hits 47663 47730 +67
- Misses 11848 11856 +8
- Partials 2400 2407 +7 ☔ View full report in Codecov by Sentry. |
dallan-keylogic
left a comment
dallan-keylogic
left a comment
There was a problem hiding this comment.
Looks good for a first pass.
| else: | ||
| # Hopefully temperature has been scaled, so we can get the nominal value of k_rxn | ||
| # by walking the expression in the constraint. | ||
| nominals = self.get_expression_nominal_values(model.arrhenius_eqn) |
There was a problem hiding this comment.
Saponification has a rather tight range of temperatures. This is probably overkill.
There was a problem hiding this comment.
For this specific case probably, but this also stands as an example of how to do it for a more general case.
| self.set_variable_scaling_factor( | ||
| model.k_rxn, | ||
| 1 / nominal, | ||
| overwrite=overwrite, | ||
| ) |
There was a problem hiding this comment.
Order of magnitude scaling is probably not appropriate here. Based on heuristic analysis, a fixed scaling factor of 1/3 would work.
| if model.is_property_constructed("reaction_rate"): | ||
| for j in model.reaction_rate.values(): | ||
| self.scale_variable_by_default(j, overwrite=overwrite) |
There was a problem hiding this comment.
If the user provides a default, we should do that, but we can get a decent scaling factor by multiplying the maximum scaling factor among the reactants provided by some period of time (I used 3600 s). (Equivalent to dividing the minimum default value by that period of time).
| def variable_scaling_routine( | ||
| self, model, overwrite: bool = False, submodel_scalers: dict = None | ||
| ): | ||
| self.scale_variable_by_default(model.flow_vol, overwrite=overwrite) |
There was a problem hiding this comment.
Is there a way for the user to provide a default value, rather than just using whatever happens to be the default default value?
There was a problem hiding this comment.
Yes, although it might not be the best way to do it. The default values are in a class attribute (dict) which users can interact with. That said, that is probably not the most obvious API for it (we could have a method to update the defaults).
bpaul4
left a comment
bpaul4
left a comment
There was a problem hiding this comment.
I have one question, which is mainly for my own understanding of the code. The changes and additions look good.
|
|
||
|
|
||
| class SaponificationReactionScaler(CustomScalerBase): | ||
| DEFAULT_SCALING_FACTORS = {"reaction_rate": 1e2} |
There was a problem hiding this comment.
How does the scaling framework know to use this value?
There was a problem hiding this comment.
When you tell it to scale_variable_by_default or scale_constraint_by_default.
5 participants
Part of new scaling deployment
Summary/Motivation:
This PR adds a new scaler for the Equilibrium Reactor unit model and the saponification property packages. It also fixes a couple of bugs found whilst writing these.
Changes proposed in this PR:
Legal Acknowledgement
By contributing to this software project, I agree to the following terms and conditions for my contribution: