Activating IPOPT_V2 with presolver #1436
Conversation
dallan-keylogic
left a comment
There was a problem hiding this comment.
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.
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.
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.
dallan-keylogic
left a comment
There was a problem hiding this comment.
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
| domain=str, | ||
| default="gradient-based", | ||
| description="Ipopt NLP scaling method", | ||
| doc="Ipopt NLP scaling method", |
There was a problem hiding this comment.
What's the difference between the doc and description fields, anyway?
There was a problem hiding this comment.
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. |
jsiirola
left a comment
There was a problem hiding this comment.
Overall, looks good. One question for @andrewlee94...
idaes/core/solvers/get_solver.py
Outdated
|
|
||
| if options is not None: | ||
| solver_obj.options.update(options) | ||
| if isinstance(solver_obj, LegacySolverWrapper): |
There was a problem hiding this comment.
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] = vfor 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.
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 oldipoptsolver interface, whilst the newInitializersand core tests will switch toipopt_v2. The default solver will remainipoptfor now until all the examples have been switched over (or updated to explicitly useipopt), at which point we will deprecate the use ofipoptas the default.Changes proposed in this PR:
writer_configargument and deprecatingoptionsforsolver_options).Initializersto useipopt_v2.idaes/coreandidaes/modelstests to useipopt_v2.Where possible, switchidaes/appsandidaes/models_extratests to useipopt_v2.Legal Acknowledgement
By contributing to this software project, I agree to the following terms and conditions for my contribution: