-
Notifications
You must be signed in to change notification settings - Fork 277
SlSeparator and Valve Scaling #1693
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
SlSeparator and Valve Scaling #1693
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## scaling_toolbox #1693 +/- ##
================================================
Coverage 77.39% 77.39%
================================================
Files 395 395
Lines 64581 64592 +11
Branches 10851 10851
================================================
+ Hits 49983 49993 +10
- Misses 12092 12093 +1
Partials 2506 2506 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| from idaes.core.solvers import get_solver | ||
| from pyomo.util.check_units import assert_units_consistent, assert_units_equivalent | ||
|
|
||
| # from pyomo.util.check_units import assert_units_consistent, assert_units_equivalent |
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.
delete
| # Unscaled | ||
| jac, _ = get_jacobian(valve_model, scaled=False) | ||
| assert (jacobian_cond(jac=jac, scaled=False)) == pytest.approx( | ||
| 5.34657e5, rel=1e-3 | ||
| ) | ||
|
|
||
| # Scaled | ||
| sm = TransformationFactory("core.scale_model").create_using( | ||
| valve_model, rename=False | ||
| ) | ||
| jac, _ = get_jacobian(sm, scaled=False) | ||
| assert (jacobian_cond(jac=jac, scaled=False)) == pytest.approx(27.270, rel=1e-3) |
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.
In the future, it might be nice wrap this in a helper function that just takes in (1) the model, (2) unscaled cond number, and (3) scaled cond number as required inputs. Tolerances for unscaled and scaled values could be optional, with default of 1e-3.
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.
Yeah. I want to rework get_jacobian to have separate options for 1) applying user scaling factors and 2) performing IPOPT's gradient-based autoscaling. Then we won't even need to do the scaling transformation (which is slow).
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.
LGTM -just a commented import to delete and a comment to consider for the future.
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.
LGTM
Summary/Motivation:
Adds scaler object for the
SLSeparatorand tests the existingPressureChangerScaleron theValveunit model.Depends on #1688.
Legal Acknowledgement
By contributing to this software project, I agree to the following terms and conditions for my contribution: