-
Notifications
You must be signed in to change notification settings - Fork 294
Change to scale_model=True #1653
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
base: scaling_toolbox
Are you sure you want to change the base?
Changes from all commits
8cdda75
c275de1
1430dcb
03e3530
00c1b58
7a41512
3567ead
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -45,6 +45,7 @@ | |
| unscaled_variables_generator, | ||
| unscaled_constraints_generator, | ||
| ) | ||
| from idaes.core.util.scaling import constraint_scaling_transform | ||
| import idaes.logger as idaeslog | ||
|
|
||
| currdir = this_file_dir() | ||
|
|
@@ -1742,6 +1743,52 @@ def test_set_scaling_factor_overwrite_false(self, caplog): | |
| ) | ||
| assert m.scaling_factor[m.v] == 10 | ||
|
|
||
| @pytest.mark.unit | ||
| def test_set_scaling_factor_transformed_constraint(self): | ||
| m = ConcreteModel() | ||
| m.x = Var(initialize=500) | ||
| m.c1 = Constraint(expr=m.x <= 1e3) | ||
| m.c2 = Constraint(expr=m.x == 1e3) | ||
| m.c3 = Constraint(expr=m.x >= 1e3) | ||
|
|
||
| def indexed_constraint_rule(b, idx): | ||
| return b.x == 1e3 | ||
|
|
||
| m.c4 = Constraint([1, 2, 3], rule=indexed_constraint_rule) | ||
|
|
||
| def match(cname): | ||
| return re.escape( | ||
| f"Attempted to set constraint scaling factor for transformed constraint {cname}. " | ||
| "Please use only one of set_scaling_factor and constraint_scaling_transform " | ||
| "per constraint to avoid double scaling." | ||
| ) | ||
|
|
||
| constraint_scaling_transform(m.c1, 1e-3) | ||
| constraint_scaling_transform(m.c2, 1e-3) | ||
| constraint_scaling_transform(m.c3, 1e-3) | ||
| for idx in m.c4: | ||
| constraint_scaling_transform(m.c4[idx], 1e-3) | ||
|
|
||
| with pytest.raises(RuntimeError, match=match("c1")): | ||
| set_scaling_factor(m.c1, 1e-3) | ||
| with pytest.raises(RuntimeError, match=match("c2")): | ||
| set_scaling_factor(m.c2, 1e-3) | ||
| with pytest.raises(RuntimeError, match=match("c3")): | ||
| set_scaling_factor(m.c3, 1e-3) | ||
| with pytest.raises( | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do you have a test of the correct way to apply scaling to
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Okay, I see now that the allowed case is also being tested. I only saw the error check on |
||
| RuntimeError, | ||
| match=re.escape( | ||
| "Attempted to set constraint scaling factor for indexed constraint c4 " | ||
| "with transformed ConstraintData children. Please use only one of " | ||
| "set_scaling_factor and constraint_scaling_transform " | ||
| "per constraint to avoid double scaling." | ||
| ), | ||
| ): | ||
| set_scaling_factor(m.c4, 1e-3) | ||
| for idx in m.c4: | ||
| with pytest.raises(RuntimeError, match=match(f"c4[{idx}]")): | ||
| set_scaling_factor(m.c4[idx], 1e-3) | ||
|
|
||
| @pytest.fixture | ||
| def model_expr(self): | ||
| m = ConcreteModel() | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 throughconstraint_scaling_transformwill have their scaled form passed to the solver. If you pass thenlp_scaling_method=user-scalingoption to IPOPT, IPOPT will use the user-provided scaling factors when computing and inverting the barrier hessian instead of its owngradient-scalingmethod. 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 thatconstraint_scaling_transformwas 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 needconstraint_scaling_transform).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 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 thanTrue?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.
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 thoseExpressionsgave 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 usingscale_model == Trueresults in double scaling, whereas withscale_model == Falsethe double scaling would be less of a problem. That's why I went through and added a bunch ofExceptionsif the user tried double scaling things.