Eliminate bounds relaxation in WaterTAP Ipopt #1162
Conversation
bknueven
requested review from
adam-a-a,
TimBartholomew,
avdudchenko,
carson-tucker and
lbibl
Codecov ReportAll modified lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1162 +/- ##
==========================================
+ Coverage 94.75% 94.89% +0.13%
==========================================
Files 341 341
Lines 34549 34524 -25
==========================================
+ Hits 32737 32760 +23
+ Misses 1812 1764 -48
☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
I think this a positive change to WT as in my tests it results in more consistent solving behavior on complex flow sheets - e.g. If it solves, it will generally solve consistently, and changing scaling, bounds etc. produces immediate effects, while with current main implementation changes to bound in scaling/initialization might help solve one state, but model would be marginally stable after wards. This is anecdotal observation rather then a quantitative one. My hypothesis on this is replacing 0 bound would allow small negative values, that could drive instability in some flow-sheets and this was primary impact on using Reals vs. NonNegativeReals -this behavior was really an issue if model was marginally scaled, or not well conditioned. I seen this directly when initializing NF-DSPM-DE model with wrong initial guess, as I would get warning of value being set below the lower bound of 0 to tiny negative value (-1e-15, or 1e-25) this some time would not impact the model and it would solve fine, some times it would break.
P.S, Looks like updates to MCAS and NF scaling made those flowsheet solve with this change.
| @@ -946,13 +946,16 @@ def run_case2( | |||
| init_options = {**solver.options} | |||
| init_options["tol"] = init_tol | |||
| init_options["constr_viol_tol"] = init_tol | |||
| init_options["ma27_pivtol"] = 5e-1 | |||
There was a problem hiding this comment.
Have we tested this as a global default? Or does that break many flowsheets?
There was a problem hiding this comment.
I have not tested it extensively. I think I will explore it as a next step.
Ipopt will increase this automatically if it thinks it's "needed", so an in-between would be to set ma27_pivtolmax higher than the current default of 1e-04.
| @@ -105,7 +105,7 @@ def test_solution(self, model): | |||
| value(model.fs.unit.inlet.flow_mass_comp[t, j]), rel=1e-5 | |||
| ) == value(model.fs.unit.outlet.flow_mass_comp[t, j]) | |||
|
|
|||
| assert pytest.approx(8.4491446e-11, rel=1e-5) == value( | |||
| assert pytest.approx(1.8449145e-10, rel=1e-5) == value( | |||
There was a problem hiding this comment.
For all intents and purposes, this is in our eyes 0, should we not typically test these as value has to be <=1e-8 ? which is linked to tolerances of our constraints?
There was a problem hiding this comment.
Yes, probably. But there are lots of small value comparisons in the ZO unit model tests, so I choose to preserve this for consistency.
bknueven
force-pushed
the
bound_relax_common_2
branch
from
5e7d621
to
4e81f89
Compare
| @@ -658,7 +658,7 @@ def build(self): | |||
| self.flowsheet().time, | |||
| self.diluate.length_domain, | |||
| initialize=0.9, | |||
| bounds=(0, 1), | |||
| bounds=(0, 1 + 1e-10), | |||
There was a problem hiding this comment.
What happened without this?
There was a problem hiding this comment.
At least one test fails, and the solves look uglier..
https://github.com/bknueven/watertap/actions/runs/6423080058/job/17440867424?pr=11
There was a problem hiding this comment.
If it were possible to eliminate these bounds that would probably be better.
| @@ -148,7 +148,7 @@ def test_mvc(): | |||
| solver = get_solver() | |||
| initialize(m, solver=solver) | |||
|
|
|||
| results = solver.solve(m, tee=False) | |||
| results = solver.solve(m, tee=True) | |||
There was a problem hiding this comment.
Should we set back to False?
There was a problem hiding this comment.
No, because pytest suppresses the output by default, and if the test fails we always want to look at the solver logs.
Which is part of the reason I left some other debugging outputs in the tests as well. I don't see any point in not having them immediately when needed.
|
Changes on ED files look ok to me. |
bknueven
force-pushed
the
bound_relax_common_2
branch
from
18a05b1
to
c449a2e
Compare
Replaces #1140
@avdudchenko reports in consistencies in the way domain=pyo.NonNegativeReals vs bounds=(0, None). I tracked this down to an issue in the way "bounds relaxation" is handled in the WaterTAP wrapper for Ipopt.
This PR would resolve this issue by turning off bounds relaxation altogether for the WaterTAP wrapper for Ipopt.
Summary/Motivation:
By default, Ipopt relaxes the bounds internally, e.g.,
(0, 1)becomes(-1e-10, 1 + 1e+10). Because of WaterTAP's use of variable scaling, theipopt-watertapwrapper was doing this operation itself. Unfortunately, the wrapper contained a bug wherein explicit bounds of(0, None)would get relaxed to(-1e-10, None)butdomain=pyo.NonNegativeRealswould not get relaxed, and would be represented as(0, None)within Ipopt.Because these should be consistent, this PR would opt to turn off bounds relaxation entirely within the
ipopt-watertapwrapper. The main affect of this is that Ipopt will always return a solution strictly inside the specified bounds. Therefore, it is generally not a good idea to specify implied bounds on variables -- a few commits (19a7dec, 3650135, 5e7d621) remove or loosen bounds on variables for model stability.Changes proposed in this PR:
bounds_relax_factor == 0to be the default behavior foripopt-watertapipopt-watertapLegal Acknowledgement
By contributing to this software project, I agree to the following terms and conditions for my contribution: