Add get_current_iterate and get_current_violations methods to Problem class
#182
Conversation
|
Thanks for the submission. We need to get #178 fixed, so these tests can run here. It is almost there. Then we can check this out. |
|
Can you merge in master here, so the update CI files will be used? |
cyipopt/cython/ipopt_wrapper.pyx
Outdated
| # Return values to user | ||
| # - Is another data type (e.g. dict, namedtuple) more appropriate than | ||
| # simply a tuple? | ||
| # - Should `ret` be returned here? |
There was a problem hiding this comment.
If ret is 0, then we should return nan for the values. That is probably easiest to interpret for a Python users.
There was a problem hiding this comment.
I've raised a RuntimeError in this case. I don't like nan as it's unclear whether it came from Ipopt or us. Return None, raise an error, or include ret in the returned object are all things I'd be okay with.
There was a problem hiding this comment.
I was imagining that there could be cases where a problem could complete, but the values weren't returned from these new methods, so you could complete the solve. But maybe that doesn't occur and ret=0 means we should stop the solve.
There was a problem hiding this comment.
The only case I know of where this returns 0 is if it is called outside of a callback. There are other branches that return false in the Ipopt code, but I don't know how to reach them. I've updated to return None in this case so that it's a little easier for a user to handle (i.e. decide whether to complete solve or not).
cyipopt/cython/ipopt_wrapper.pyx
Outdated
|
|
||
| # Return values to user | ||
| # - Is another data type (e.g. dict, namedtuple) more appropriate than | ||
| # simply a tuple? |
There was a problem hiding this comment.
In .solve() we return a tuple of an ndarray (optimal solution) and a dict mapping to other arrays including the optimal solution again. I can't say that is the cleanest API. But maybe we should mimic it here, since it is similar information?
There was a problem hiding this comment.
We could mimic this for get_current_iterate, but I don't see what the first ndarray should be for get_current_violations, and I think it's more important for these to be consistent with each other than with solve. I'm currently returning dicts for both methods. In get_current_iterate, the keys are a subset of those in solve's returned dict. In get_current_violations, they match the GetIpoptCurrentIterate documentation, although I'm thinking about changing "nlp_constraint_violation" -> "g_violation".
|
There are two ways a user can create problems. The first should work fine with your new methods: class MyProblem(Problem):
def __init__(self):
self.iterations = []
self.pr_violations = []
self.du_violations = []
def intermediate(self, *args):
x, _, _, _, _ = self.get_current_iterate()
self.iterations.append(x)
_, _, _, _, grad_lag, g_viol, _ = self.get_current_violations()
self.pr_violations.append(g_viol)
self.du_violations.append(grad_lag)
mp = MyProblem()
mp.solve()We can decide to only support this way if you want to get the intermediate values. But I think this may work: class MyProblem:
def __init__(self):
self.iterations = []
self.pr_violations = []
self.du_violations = []
def intermediate(self, *args):
x, _, _, _, _ = self.get_current_iterate()
self.iterations.append(x)
_, _, _, _, grad_lag, g_viol, _ = self.get_current_violations()
self.pr_violations.append(g_viol)
self.du_violations.append(grad_lag)
mp = MyProblem()
p = Problem(problem_obj=mp)
mp.get_current_iterate = p.get_current_iterate
mp.get_current_violations = p.get_current_violationsIf we made class MyProblem:
def __init__(self):
self.iterations = []
self.pr_violations = []
self.du_violations = []
def intermediate(self, *args):
x, _, _, _, _ = get_current_iterate(self.prob)
self.iterations.append(x)
_, _, _, _, grad_lag, g_viol, _ = get_current_violations(self.prob)
self.pr_violations.append(g_viol)
self.du_violations.append(grad_lag)
mp = MyProblem()
p = Problem(problem_obj=mp)
mp.prob = p
p.solve()The last two options are not ideal, but may work. I'm also fine just not supporting that setup. |
|
Regarding the three options you present:
|
|
Since these new methods are only present in ipopt >= 3.14, we need to disable them for prior versions. We technically support back to 3.11, I believe. Certainly 3.12. |
|
We probably have to check at build time for < 3.14. |
I have this working locally using macros, following the example in the Cython documentation. Not sure if you had something else in mind that would be preferred. |
|
mac is giving this conda error: |
Any idea what could be causing this? I see no pattern to which mac builds succeed or fail. |
…l infeasibility to 1e-8
I just switched to libmamba as a work around for another conda bug, but maybe this just exposes a new one. I'm not sure either. |
|
This may be the mac issue: mamba-org/mamba#1826 |
|
Pyomo/pyomo#2760 has been merged, so I removed all the code that was necessary to support a 12-argument callback (to access @moorepants this is ready for re-review/merge when you have a chance. |
|
There are some errors, e.g.: |
|
Looks like this is a known issue. See conda-forge/scipy-feedstock#224 and scipy/scipy#18371. |
|
I changed it to which seems like the last < is being picked up as a bash command even though I have it in quotes. |
|
This is stated in the conda docs: "When using the command line, put double quotes around any package version specification that contains the space character or any of the following characters: <, >, *, or |." |
|
Sorry, I added several commits. Seems like this may be a mamba bug on the version specification. |
|
This gives a clearer error message if scipy is pinned to 1.10.1, I guess there are no versions for Python 3.7: If I use |
|
It really needs to be |
|
Python 3.7 is end of life in 2 months. So we can drop it for the next release. Maybe the simplest solution is to drop Python 3.7 here. |
|
This looks good to me. Thanks for the large effort getting this. It is a nice new addition! |
Fixes #180
Being able to access primal/dual variable values as well as Ipopt's computed infeasibilities during a callback would be very useful for debugging NLPs. This PR contains a basic implementation of methods that call
GetIpoptCurrentIterateandGetIpoptCurrentViolations. These methods can be called from an intermediate callback if the callback is somehow aware of theProblem.This PR is marked as WIP as the methods currently segfault if not called from a callback. Ideally this would raise a Python exception, but I'm not sure what is the best way to detect whether we are "in a callback". Maybe a global flag in ipopt_wrapper.pyx?Edit: The problem is calling
intermediate_cbwhen we are not in an IpoptSolve. We use a flag to detect this and raise an error.Edit: Alternatives
Admittedly, the callback needing to be aware of the Problem object is somewhat clunky, but this is the best solution I could think of that doesn't involve changing the callback API. If we were willing to change the callback API, we could:
intermediate_cbto the user-facing callbackCallGetIpoptCurrentIterateandGetIpoptCurrentViolationsfromintermediate_cb, then return the results to the user's callback somehow. This would require accessing__nlpfromintermediate_cb, however, which I don't know how to do.Problemand implementintermediateas a method, which allows us to access the new methods from the callback without altering its call signature.inspect.signatureto detect how many positional arguments are accepted by the user's callback, and send theProblemobject to the callback if the right number (12 or*args) are accepted. For anything other than 11 or 12 positional arguments or*args, an exception is raised.