-
Notifications
You must be signed in to change notification settings - Fork 238
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
Activating IPOPT_V2 with presolver #1436
Conversation
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.
Overall this looks good. Eagerly awaiting the rollout of the new scaling tools, though. Can we manually activate the solve-time scaling by passing a solver writer option?
CONFIG.block_solver_options.declare( | ||
"tol", | ||
ConfigValue( | ||
default=1e-8, | ||
domain=float, | ||
description="Convergence tolerance for block solver", | ||
), | ||
) |
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.
Might be good to set constr_viol_tol
as well. I'm not sure what tol
does in square problems, but in optimization problems constr_viol_tol
controls the constraint violation.
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.
Whilst most tests appear to pass if we change this to 1e-6, there are a few that start failing so I will leave this out for now. Users can always set this value if they wish.
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.
Looks good, although it would still be nice to keep around the solve_strongly_connected_components
variant of the initializer to use as a debugging tool. Making it a config option instead of a totally different initializer is preferable because we don't want to have to maintain an additional, parallel routine that's 90% the same.
idaes/config.py
Outdated
@@ -346,7 +344,6 @@ def _new_idaes_config_block(): | |||
domain=str, | |||
default="gradient-based", | |||
description="Ipopt NLP scaling method", | |||
doc="Ipopt NLP scaling method", |
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.
What's the difference between the doc
and description
fields, anyway?
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.
Long (doc
) and short (description
) doc strings - which one shows up depends on exactly what you use to display the config block/value.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1436 +/- ##
==========================================
- Coverage 77.83% 77.73% -0.10%
==========================================
Files 394 394
Lines 64926 64953 +27
Branches 14398 14404 +6
==========================================
- Hits 50535 50494 -41
- Misses 11807 11879 +72
+ Partials 2584 2580 -4 ☔ View full report in Codecov by Sentry. |
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.
Overall, looks good. One question for @andrewlee94...
idaes/core/solvers/get_solver.py
Outdated
if solver is None: | ||
solver = "default" | ||
solver_obj = idaes.core.solvers.SolverWrapper(solver, register=False)() | ||
|
||
if options is not None: | ||
solver_obj.options.update(options) | ||
if isinstance(solver_obj, LegacySolverWrapper): |
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.
It is unfortunate that we need to have this test here: the whole point of the Legacy interface is that you shouldn't have to change your code. Is there a reason why you can't just do:
if options is not None:
for k, v in options.items():
solver_obj.options[k] = v
if writer_config is not None:
for k, v in writer_config.items():
solver_obj.config.writer_config[k] = v
for both new and old solvers? If there writer_config is not None for an old solver, it should just generate an error when you try to set the value on config.writer_config
...
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.
I can definitely try (and this is why I was waiting for your review).
Replaces #1415
Summary/Motivation:
Second attempt at implementing new solver interface with presolve. Due to the need to maintain backward compatibility, the old
model.initialize()
methods will continue to use the oldipopt
solver interface, whilst the newInitializers
and core tests will switch toipopt_v2
. The default solver will remainipopt
for now until all the examples have been switched over (or updated to explicitly useipopt
), at which point we will deprecate the use ofipopt
as the default.Changes proposed in this PR:
writer_config
argument and deprecatingoptions
forsolver_options
).Initializers
to useipopt_v2
.idaes/core
andidaes/models
tests to useipopt_v2
.Where possible, switchidaes/apps
andidaes/models_extra
tests to useipopt_v2
.Legal Acknowledgement
By contributing to this software project, I agree to the following terms and conditions for my contribution: