Change to scale_model=True #1653
Conversation
… user double-scales model
dallan-keylogic
requested review from
agarciadiego,
bpaul4,
ksbeattie and
lbianchi-lbl
as code owners
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## scaling_toolbox #1653 +/- ##
==================================================
Coverage ? 77.04%
==================================================
Files ? 395
Lines ? 63939
Branches ? 10512
==================================================
Hits ? 49264
Misses ? 12195
Partials ? 2480 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
idaes/core/util/scaling.py
Outdated
| "Attempted to set constraint scaling factor for indexed constraint " | ||
| "with transformed ConstraintData children. Please use only one of " |
There was a problem hiding this comment.
| "Attempted to set constraint scaling factor for indexed constraint " | |
| "with transformed ConstraintData children. Please use only one of " | |
| f"Attempted to set constraint scaling factor for indexed constraint {c.name}" | |
| f"with transformed ConstraintData children. Please use only one of " |
I think it could be useful to directly point to the constraint where the issue arises, in the case that the user might not be applying scaling to constraints manually, one at a time.
idaes/core/util/scaling.py
Outdated
| scaling_factor = sfFinder.find(component_data=c) | ||
| if scaling_factor is not None: | ||
| raise RuntimeError( | ||
| "Attempted to transform constraint with existing scaling factor. " |
There was a problem hiding this comment.
| "Attempted to transform constraint with existing scaling factor. " | |
| f"Attempted to transform Constraint {c.name} with existing scaling factor. " |
There was a problem hiding this comment.
Again, it'd be nice to call out the exact constraint with the issue when the user is not carefully scaling constraints one at a time.
| pyomo.common.config.ConfigValue( | ||
| domain=Bool, | ||
| default=False, # TODO: Change to true once transition complete | ||
| default=True, |
There was a problem hiding this comment.
high-level: what happens when we do not set scale_model to True by default? At this point, I am a bit mixed up on what's happening with regard to scaling and how the solver will "see" things.
There was a problem hiding this comment.
When scale_model == False, then scaling factors are not applied to the model at the time an .nl file is written for the solver. Constraints transformed through constraint_scaling_transform will have their scaled form passed to the solver. If you pass the nlp_scaling_method=user-scaling option to IPOPT, IPOPT will use the user-provided scaling factors when computing and inverting the barrier hessian instead of its own gradient-scaling method. However, it will not use scaled values of the variables when computing the distance of a variable to its bounds or when computing the constraint residual (the latter fact being the reason that constraint_scaling_transform was created).
Now that scale_model==True, solvers that support the new .nl writer (definitely IPOPT, I'm not sure about other solvers) will only see the scaled model. The Jacobian will be computed using the scaled variables, and IPOPT will apply its gradient-based scaling on top of the user scaling factors (which is a good thing). Scaled values of variables will be used to compute the distance to bounds and scaled values will be used in computing the constraint residual (so we no longer need constraint_scaling_transform).
There was a problem hiding this comment.
In light the benefits described in your second paragraph, why would one want to set scale_model == False? Is there a situation when that would be more useful than True?
There was a problem hiding this comment.
Prior to Pyomo 6.9.3, there was a bug in how scaling factors were applied in linear named Expressions---namely, they weren't applied in such expressions. Because those Expressions gave incorrect values, most models in the IDAES repo either failed to solve or gave erroneous results. It was fixed in this release, which is why I'm making this change now.
The only situation where you wouldn't want to scale the model is if you suspect a constraint has both a scaling factor set and has been transformed through constraint_scaling_transform. Then using scale_model == True results in double scaling, whereas with scale_model == False the double scaling would be less of a problem. That's why I went through and added a bunch of Exceptions if the user tried double scaling things.
| set_scaling_factor(m.c2, 1e-3) | ||
| with pytest.raises(RuntimeError, match=match("c3")): | ||
| set_scaling_factor(m.c3, 1e-3) | ||
| with pytest.raises( |
There was a problem hiding this comment.
Do you have a test of the correct way to apply scaling to c4?
There was a problem hiding this comment.
I'm afraid I don't follow. I test applying a scaling factor both to the constraint as a whole as well as the individual indices. What other case do you want to test?
There was a problem hiding this comment.
Okay, I see now that the allowed case is also being tested. I only saw the error check on c4 before.
adam-a-a
left a comment
adam-a-a
left a comment
There was a problem hiding this comment.
LGTM - just some pylint test failing
4 participants
Summary/Motivation:
Changes
scale_modeltoTruein the .nl writer config to enable IDAES to take advantage of writer-time scaling. Additionally,Exceptionshave been added when the user attempts to useset_scaling_factoron a constraint that has already been transformed usingconstraint_scaling_transformand, conversely, when the user attempts to useconstraint_scaling_transformon a constraint which already has a scaling factor.Legal Acknowledgement
By contributing to this software project, I agree to the following terms and conditions for my contribution: