-
Notifications
You must be signed in to change notification settings - Fork 562
Use projected NLPs for the inner problem solves in ExternalPyomoModel #2283
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
Conversation
Codecov Report
@@ Coverage Diff @@
## main #2283 +/- ##
==========================================
- Coverage 85.29% 85.21% -0.09%
==========================================
Files 611 616 +5
Lines 76006 77925 +1919
==========================================
+ Hits 64833 66407 +1574
- Misses 11173 11518 +345
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
… test that mocks unavailable
| else: | ||
| with TemporarySubsystemManager(to_fix=inputs): | ||
| solver.solve(block) | ||
| if self._use_cyipopt: |
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.
elif to avoid the extra indentation?
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.
See comment below. I think I want to have this extra if statement so that the increment of vector_scc_idx happens regardless of whether we're using CyIpopt.
| with TemporarySubsystemManager(to_fix=inputs): | ||
| solver.solve(block) | ||
|
|
||
| vector_scc_idx += 1 |
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 think this is technically correct as written, but it would be clearer if it was done inside the if self._use_cyipopt block.
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 think I like having this outside the if self._use_cyipopt tree, as otherwise, in the case where we use a regular Pyomo solver, vector_scc_idx will always be zero, even though we may not be at the zeroth "vector-valued" SCC. Having this information may be useful, e.g. for debugging, even if it's not strictly necessary for the algorithm.
Summary/Motivation:
There is a significant performance improvement for some problems if we create ProjectedNLPs that we solve repeatedly with CyIpopt, rather than solve Pyomo blocks with Ipopt, for the inner problems in the implicit function formulation.
Changes proposed in this PR:
A comparison comparison on a chemical looping steady state optimization problem:
Legal Acknowledgement
By contributing to this software project, I have read the contribution guide and agree to the following terms and conditions for my contribution: