MAINT: optimize: make trust-constr accept constraint dict (#9043) #9112
Conversation
|
The other direction should also be added, old solvers accepting the "new" constraint format. |
scipy/optimize/_minimize.py
Outdated
| @@ -585,6 +587,23 @@ def minimize(fun, x0, args=(), method=None, jac=None, hess=None, | |||
| if isinstance(bounds, Bounds): | |||
| bounds = new_bounds_to_old(bounds.lb, bounds.ub, x0.shape[0]) | |||
|
|
|||
| all_constraint_types = (NonlinearConstraint, LinearConstraint, dict) | |||
There was a problem hiding this comment.
This should go to a separate helper function, maybe called
normalize_constraints_to_new_format, only the if method == 'trust-constr': (...) = normalize_constraints_to_new_format(...) left here.
There was a problem hiding this comment.
Also, the bounds would need to be normalized.
There was a problem hiding this comment.
@pv You've written:
"Also, the bounds would need to be normalized."
and in the original issue:
"Automatic conversion of the "old-style" ... bounds specifications to the "new" format"
I think the code ending two lines above your comment already addresses this. The new solver accepts old-style bounds, and the old solvers accept new-style bounds. The tests I wrote use old-style bounds with the new solver. What do you mean when you write that the bounds needs to be normalized/converted? It seems this was already done.
I did want to put this all in a separate function, but instead I followed the style of the bounds conversion code and left it inline. I suppose I'll move both to separate functions in _constraints.py.
If I'm going to do this, I'd prefer to take all of the logic out and just write functions:
bounds = standardize_bounds(bounds, method)
constraints = standardize_constraints(constraints, method)
which makes the bounds/constraints appropriate for the method.
There was a problem hiding this comment.
Regarding conversion in the other direction, you mentioned in the original issue that an error message would also be acceptable "if [conversion is] not possible". I took "if that's not possible" to mean if it is not implemented. It's not implemented, so I raised an error. I would prefer to leave that for a separate PR because I did not originally intend to implement it and it will be a bit more involved. This PR is for making "trust-constr accept constraint dict".
|
I can't go any further with this until I can get my development setup fixed. Thoughts on how to fix? |
|
"rm -rf build"? If it's not that, probably something with compiler/libraries setup is wrong, but I don't know about osx. |
|
It seems to be working after installing both gcc 7 and gcc 8 and moving some folders around... |
|
@pv "The other direction should also be added..." Equality constraints and inequality constraints must be separated in the old constraint dictionaries, however they are (in general) specified together in the new constraint classes - an equality constraint is when the new-style constraint lower bound and upper bound are equal. I see three options:
The latter might sound appealing, but without knowing the details of SLSQP, I'm not sure whether that will work very well in practice. (I'm assuming that strategy wouldn't work well for COBYLA, otherwise why doesn't it accept equality constraints and automatically convert them into pairs of inequality constraints?) I repeatedly expressed my concerns over abandoning the old-style constraint dictionaries during the development of |
|
The current API inconsistency I think makes no sense, and needs to be addressed. I regret I did not pay close attention to the details here at an earlier time, but it is still possible to fix this. It is simplest to take the naivest option (e.g. evaluate the function many times if needed). Since the user can specify a list of Constraint objects, the performance issue can be avoided if necessary. There are two options: convert only those Constraint objects to equality constraints where all of the bounds are equal, or to try to do the conversion piecemeal. The simpler option is likely the former. If the user wants equality constraints, those need to be specified in a seprate Constraint object insofar the "new" API is concerned. EDIT: also the option of evaluating multiple times and picking out the right parts by indexing is probably fine, doesn't probably need that much code. The "raise error on wrong way to specify constraints" is not good, because the whole point of the minimize API was to provide an unified way to specify and solve optimization problems. This idea should be still followed, as the whole thing makes no sense otherwise, and reasons for keeping the discrepancy appear not strong. Performance is a secondary concern, and as noted above, there does not appear to be an unavoidable performance cost implied. |
|
Ok, I'll fix it. Yeah I planned to separate them with arbitrary mix. Yay fancy indexing. |
|
Thanks! It's of course possible to split the problem to multiple PRs (or tell me to do it myself :), if you like. |
|
@pv I think all the new to old constraint conversion functionality is there except conversion of constraint Jacobian. I waited because in the specification for new-style nonlinear constraints, the user can specify an approximation strategy rather than a callable. I see three options:
The other question is what to do about COBYLA. Right now none of the tests (written with SLSQP in mind) pass using COBYLA. Some of this is because COBYLA can't handle bounds (which really should be automatically converted to inequality constraints, right?) or equality constraints; other times COBYLA converges to a different local minimum (presumably) or doesn't converge. Not sure what to do here. Suggestions for additional tests would be welcome! |
|
First of all, thank you very much for taking the initiative and helping fixing this @mdhaber! The next diagram summarizes the current status of how
As pointed out in issue #9043, we don't have a direct arrow between new constraint classes and old constraint classes, and this goes against the entire principle of I would like to point out that I see two basic approaches here:
The second solution looks more elegant and efficient to me. It would also avoiding repeating code in different constrained optimizer. Nevertheless it has the dangerous I just mentioned. Probably the best path to take here is to finishing merging this PR implementing the strategy (1), accepting eventual inefficiencies and redundancies in the code. In a latter PR, if we want to solve these inefficiencies and improve the code quality, we implement (2). What do you think Matt? |
|
@nmayorov I think your opinion here would be useful as well |
|
Sure, reworking SLSQP and COBYLA so that they use the new constraint classes sounds best (if done perfectly). To maintain backwards compatibility, you could leave in their existing constraint handling code, too, or you could use the old-to-new converter that's part of this PR, which is pretty efficient. But that sounds like a lot of work. I was trying to do something non-invasive that I could finish last weekend. So I think I'd be happy to finish this PR right now and leave all that to future work, unless you are offering to do what you propose in time for 1.2. If not, the big question I need answered is how we would prefer to transfer Jacobian specifications from new-to-old per my comment above. In that case, I'd appreciate your thoughts on that, and let's move discussion of a future PR to a new issue |
|
I agree with you: it is best to finish this PR now and leave harder modifications, as the ones I just described, for latter PR (to be integrated in scipy >=1.3). About the jacobian: I think the best option is : |
|
Yes, I agree it would be better to modify SLSQP and COBYLA to work with the new constraint format directly. However, we'd still need the old->new format conversion code. Revising SLSQP/COBYLA is likely not that hard in the end, as the inputs to the fortran routines are well-defined (and hence it's not directly a modification of the algorithmic code, so I'm hoping there are no weird interactions). Constraint Jacobians: SLSQP uses naive numerical differentiation implemented on the Python side if user-provided routine is not given, and COBYLA doesn't need the jacobian. As such, there's no need to allow using the "default numerical jacobian", as what's implemented in trust-constr is likely better. So option 2) is probably better for the new->old conversion. |
|
On COBYLA: for this PR, I'd suggest skipping tests for For convergence to different solutions --- if they're local minima, maybe multiple solutions can be allowed, or you can try switching to more trivial test problems for this PR. |
|
Matt, if you decide to go for (2) it is probably better if you take a look on the class PreparedConstraints, that is where the numerical diferentiation is implemented... |
|
@pv you write: Are you agreeing that I should complete this PR and leave reworking the other optimizers for future work? @antonior92 thanks, I know. That's what I had in mind. |
|
I mean as a long term goal, it would be better to use only one format internally. |
|
@antonior92 I tried using the For example, if Correct? Well before I even got to using Any thoughts on why that would be? What is going on inside the |
|
Hi Matt, Actually, I introduce some memoization logic inside "PreparedConstraint" (take a look here). This is necessary, so I can efficiently approximate the derivatives (without function recomputation). This is probably responsible for the problem... I didn't found the exact source of the problem though... |
|
I read that and figured that sort of thing might be going on. The behavior of SLSQP in this situation is similar to what I've seen it do when the constraint function is unsuitable for finite difference derivative approximation. I am not using the attributes, only the public methods, so based on the two bullet points in the docstring I thought I would not be affected by any peculiarities. I seemed to be affected nonetheless, so without luck passing only the Without diving into the innards of SLSQP and COBYLA to see how they work, I need to provide them functions for evaluating the constraint and it's Jacobian for arbitrary inputs, without any unusual behavior. When I call the function with a given argument, it returns the appropriate information. Very simple. How do I persuade a |
Matt, I will need to take a look. Because I thought Probably the best way to fix this is to find the situation when the |
|
I already tried a for loop that passed random arrays into But clearly it's not working inside SLSQP. Could it be something to do with the values SLSQP passes in, maybe due to the small changes when it's trying to approximate derivatives? Do you do something like use np.allclose to detect whether the input is the same as before and used the cached value instead of re-evaluating? If you don't know, at some point I can get back to this and perform a proper test that compares the original function w/ |
I don't think this is the problem, because I only reuse the value when I have an exact match (np.array_equal).
Ok, I think we will need to be able to identify exactly when the mismatch happens. Then I can debug it and fix the problem on My guess is that it has probably something to do with the order we call the functions... |
|
If you tell me how to import |
|
Never mind again. You should checkout my branch. (I can't get a stand-alone example working properly. It's working even more bizarrely.) Look what's going on in |
|
@antonior92 Similar issue with @pv Issues with the |
|
I think I didn't do that very elegantly. Oops. |
|
hahahaha I have done the same thing once... It seems so easy. But it end up merging the master into your branch. |
|
@pv I think think this is ready for comments. |
|
|
||
|
|
||
| def new_constraint_to_old(con, x0): | ||
|
|
There was a problem hiding this comment.
Can you add a one-sentence (or so) docstring stating what the function does, even if this is in principle clear from the name.
|
|
||
|
|
||
| def old_constraint_to_new(ic, con): | ||
|
|
There was a problem hiding this comment.
One-sentence docstring, and similarly to the other functions.
| old_constraints = new_constraint_to_old(con, x0) | ||
| constraints[i] = old_constraints[0] | ||
| additional_constraints = old_constraints[1:] | ||
|
|
There was a problem hiding this comment.
I'd rewrite this as
if meth == 'trust-constr':
for i, con in enumerate(constraints):
if not isinstance(con, new_constraint_types):
constraints[i] = old_constraint_to_new(i, con)
else:
for i, con in enumerate(list(constraints)):
if isinstance(con, new_constraint_types):
old_constraints = new_constraint_to_old(con, x0)
constraints[i] = old_constraints[0]
constraints.extend(old_constraints[1:])
return constraints
Note that it is allowed to modify list contents during enumerate (but adding new items should be avoided).
Stylistically, it's better to have the meth == ifs on top level since the operations are different.
There was a problem hiding this comment.
@pv agree with meth== on top level so that the branching only has to happen once.
But I don't understand how your code constraints.extend(old_constraints[1:]) agrees with your comment "...adding new items should be avoided". I had the additional_constraints list that gets extended (via +=) only after the loop is done because I wanted to avoid adding new items during the loop.
Which would you prefer?
(in any case, thanks for pointing it out, because it looks like there was a bug in my code here. additional_constraints = old_constraints[1:] should have been additional_constraints.extend(old_constraints[1:]))
There was a problem hiding this comment.
Also, why did you write for i, con in enumerate(list(constraints)): rather than constraints = list(constraints)? I wanted to ensure that constraints is a (mutable) list as I imagine the user could have passed in some other sort of sequence of constraints.
There was a problem hiding this comment.
It's somewhat simpler to iterate over a copy of the list and mutate the original,
rather than iterating over the original and mutating the copy.
The two are not the same:
for i, cons in enumerate(list(constraints)):
# mutate constraints
and
constraints = list(constraints)
for i, cons in enumerate(constraints):
# mutate constraints
There was a problem hiding this comment.
I know they're different.
That approach makes sense.
I didn't see before that we were using list (constraints) in different ways. I was using it to ensure that it is mutable. You were using it to create a copy to iterate over. I think we actually need both.
Thanks!
| return NonlinearConstraint(fun, lb, ub, jac) | ||
|
|
||
|
|
||
| def standardize_bounds(bounds, x0, meth): |
There was a problem hiding this comment.
These two routines should be preferably moved to _minimize.py.
The meth names should preferably only be referred to there.
| jac = lambda x: A | ||
|
|
||
| # when bugs in VectorFunction/LinearVectorFunction are worked out, use | ||
| # pcon.fun.fun and pcon.fun.jac. Until then, get fun/jac above. |
|
I'll wait for CIs then merge. |
|
Ok. After merge, I'll get my branch up to date with SciPy master and address your comments in a new pr? |
|
Thanks, merged! Yes, maybe easiest to do remaining things in separate PR. |
|
Re: the strange behavior with SLSQP. SLSQP modifies the This is a bit bad behavior, and probably one should change here https://github.com/scipy/scipy/blob/master/scipy/optimize/slsqp.py#L379 and similarly |
|
Or better instead, at https://github.com/scipy/scipy/blob/master/scipy/optimize/slsqp.py#L426 |
Attempt to address #9043: this PR adds conversion from old- to new-style constraints for
trust-constrand raises error when new-style constraints are used with other methods.As far as I can tell,
trust-constralready accepts old-style bounds and converts new- to old-style bounds for other methods.Couldn't run tests on my computer due to SciPy install issues... lets see how this goes.