-
Notifications
You must be signed in to change notification settings - Fork 518
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
Refactor ExternalPyomoModel to allow different implicit function solvers #2652
Conversation
…t pass due to no support for SolverFactory solvers
…to subsolvers in scipy wrapper
from pyomo.util.subsystems import create_subsystem_block | ||
|
||
|
||
class PyomoImplicitFunctionBase(object): |
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.
Why is this function defined here? It seems a bit unrelated to (and more general than) square solvers.
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.
Good point. I think I originally had it here before I decided to make an implicit_functions.py file.
@@ -77,14 +169,18 @@ def solve(self, x0=None): | |||
|
|||
def evaluate_function(self, x0): | |||
# NOTE: NLP object should handle any caching | |||
self._timer.start("function") |
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.
You are adding logging here, but removing it in solvers/scipy_solvers.py
. Is this just leftover debugging?
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.
This was intentional. I wanted to be able to distinguish between time spent in function versus derivative evaluation when switching between Newton and Secant methods. However, this is admittedly a very low-level and frequently called method to add a timer call to. Here is comparison showing the speedup when this call is removed:
I think this is a sufficient difference that these calls should be removed.
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.
However, note that in these examples, Newton solves are performed very frequently on very small systems, which makes the function/derivative methods particularly performance-critical. I'm not sure this changes the calculus about whether to include timing calls in low-level methods.
@@ -276,74 +223,27 @@ def equality_constraint_names(self): | |||
return ["residual_%i" % i for i in range(self.n_equality_constraints())] | |||
|
|||
def set_input_values(self, input_values): | |||
self._timer.start("set_inputs") |
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.
Is this timing from debugging? Again, we seem to be inconsistent in pynumero where we are instrumenting timing and where we are removing it. I wonder if it should all be removed (pynumero is meant to be efficient, and timer adds (not completely insignificant overhead).
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.
My motivation in the timing calls is so that timing statistics like this:
Identifier ncalls cumtime percall %
------------------------------------------------------------
root 1 11.990 11.990 100.0
-------------------------------------------------------
__init__ 10 4.688 0.469 39.1
---------------------------------------------
PyomoNLP 10 2.881 0.288 61.5
other n/a 1.807 n/a 38.5
=============================================
hessian 100 1.447 0.014 12.1
jacobian 110 0.625 0.006 5.2
set_inputs 130 4.572 0.035 38.1
---------------------------------------------
solve_nlp 48230 4.049 0.000 88.6
other n/a 0.522 n/a 11.4
=============================================
other n/a 0.658 n/a 5.5
=======================================================
============================================================
can be generated without intrusive modification. I think for time-consuming methods that we might commonly want to profile, the timing calls are justified. See above for a performance diff when timing is introduced in small, fast methods that are called often, which suggests that they should not be a permanent feature of these methods.
@Robbybp I just tried using
At first glance it looks like the scipy solver interfaces are missing the |
@andrewlee94 You can try by branch in #2668 |
Summary/Motivation:
I would like to be able to test the implicit function implementation in
ExternalPyomoModel
with different solvers for the square system. This PR allows this by defining an implicit function base class, a subclass of which can be passed toExternalPyomoModel
via thesolver_class
argument.These updates allow the following performance comparisons to be made:
fsolve
function for higher dimension systems:Comparing timing profiles between the two implementations on the CLC dynamic optimization, we can see that the speedup is driven by the solve of the square NLPs.
With the original implementation:
and with
fsolve
for the higher dimension square systems:calculate_variable_from_constraint
for a SciPy scalar Newton solver (here the secant-Newton hybrid). Both implementations here usefsolve
for higher-dimension square systems:Comparing timing profiles, the slowdown is due to the amount of time spent constructing extra
PyomoNLP
s in the latter case and the fact SciPy's scalar Newton/secant solver (withPyomoNLP
's ASL interface for function/derivative evaluations) seems to be slower thancalculate_variable_from_constraint
.With
calculate_variable_from_constraint
:and with the secant-Newton wrapper around SciPy's scalar Newton solver:
These time-profile comparisons use the utilities in #2651, but this branch does not depend on that PR.
Changes proposed in this PR:
PyomoImplicitFunctionBase
defining an API for implicit function solvers that can be used byExternalPyomoModel
ImplicitFunctionSolver
andSccImplicitFunctionSolver
that implement this API with and without a strongly connected component decompositionNlpSolverBase
for NLP solvers that can be used by the above implicit function solversCyIpoptSolverWrapper
andScipySolverWrapper
, that implement this API with their respective solvers for the NLPExternalPyomoModel
to accept an implicit function solver via thesolver_class
(andsolver_options
) argument instead of interfacing directly to the NLP solver (and implementing the sequential SCC solve itself)Note that this PR removes the
use_cyipopt
argument and also removes the ability to use a "Pyomo solver" (e.g.pyo.SolverFactory('ipopt')
) withinExternalPyomoModel
(unless such a solver is implemented as a subclass ofPyomoImplicitFunctionBase
, which I have not done). While this was useful functionality for testing in the case where CyIpopt was not available, the SciPy square solvers can now be used instead, which should always be available as SciPy is a hard dependency of most of this machinery. Also worth noting that working around the CyIpopt-not-available case was always a bit contrived, as CyIpopt is (currently) the only solver thatExternalPyomoModel
works with!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: