ControlVolume0D Scaler#1651
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## scaling_toolbox #1651 +/- ##
==================================================
Coverage ? 77.03%
==================================================
Files ? 395
Lines ? 63957
Branches ? 10501
==================================================
Hits ? 49269
Misses ? 12211
Partials ? 2477 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
dallan-keylogic
requested review from
agarciadiego,
bpaul4 and
lbianchi-lbl
as code owners
bpaul4
left a comment
bpaul4
left a comment
There was a problem hiding this comment.
@dallan-keylogic this is largely good to me, just a couple typos and a question. I encountered a bunch of merge conflicts when I created a dummy branch to test against my in-progress CSTR scaling, so I'll come back with more comments or an approval one I've sorted that out.
| def test_scale_variable_by_definition_constraint_indexed(self, model): | ||
| sb = CustomScalerBase() | ||
|
|
||
| # The fact that this constraint overdetermines the mode is |
There was a problem hiding this comment.
Because an exception is thrown before the model can even be solved.
spelling fixes Co-authored-by: Brandon Paul <[email protected]>
| # Set scaling factors for element balance variables | ||
| # TODO |
There was a problem hiding this comment.
Are these TODOs going to be completed in this PR or a subsequent PR?
| for idx in model.material_balances: | ||
| self.scale_constraint_by_nominal_value( | ||
| model.material_balances[idx], | ||
| scheme=ConstraintScalingScheme.inverseMaximum, |
There was a problem hiding this comment.
Is it true that we'll just use inverseMaximum in most cases?
There was a problem hiding this comment.
I'm using it here because I already had set scaling factors for all the variables/expressions that appear in the constraint. The real thing dictating the scale here is the scaling of get_material_flow_terms, which is inherited by the rest of the mass transfer terms.
* rescue files from overloaded git branch * fix due to api tweaks * run black * forgot to add a file * fix test errors * pin coolprop version * update version, disable superancillaries * run black * respond to Marcus's feedback * getting close * address Will's comments * tests for set_scaling_factor * support for unions in python 3.9 * testing the scaling profiler is way too fragile * modify test to be less fragile * remove pdb * rescue files from branch * towards scaling cv * preliminary testing * scaling by defn constraint * scale constraint by definition constraint * Disable obsolete tests for now * run black * actually add tests * inh * tests for methods rescued from old scaling tools * pylint * test to make sure that value is reverted * Apply suggestions from code review spelling fixes Co-authored-by: Brandon Paul <[email protected]> * additional clarity * more files rescued from branch --------- Co-authored-by: Brandon Paul <[email protected]>
4 participants
Summary/Motivation:
Adds a scaler for the
ControlVolume0Din addition to another scaling helper methodChanges proposed in this PR:
ControlVolume0DandControlVolume1DControlVolume0DScalerscale_variable_by_definition_constraintsoec_design.pyRE Design of SOFC model based on SOEC unit present in idaes, issue with DOF #1631Legal Acknowledgement
By contributing to this software project, I agree to the following terms and conditions for my contribution: