search

mechmotum / cyipopt

Cython interface for the interior point optimzer IPOPT
Eclipse Public License 2.0
229 stars 54 forks source link

ENH: minimize_ipopt: add input validation for `bounds` and support for new-style constraints #207

Closed mdhaber closed 12 months ago

mdhaber commented 1 year ago

Followup to gh-206. Adds input validation for the bounds parameter of minimize_ipopt and support for scipy.optimize.Bounds objects.

mdhaber commented 1 year ago

I think I'd like to remove Bounds from this. There is enough here without it, and I want to make sure that the new-style constraints (LinearConstraint, NonlinearConstraint) can be integrated smoothly before adding Bounds. I added the constraints here, instead.

I could still use feedback on the rest, and I can address it all together.

mdhaber commented 1 year ago

@moorepants This looks like a crazy PR, but I don't think it's actually as bad as it looks. The vast majority of the lines are tests copy-pasted from SciPy.

Almost all the changes to minimize_ipopt are in the first commit, so I'd suggest reviewing that first - https://github.com/mechmotum/cyipopt/pull/207/commits/9a01fa627c2fbda97b026b3bea1aef292a44eca1. I am happy with it, but there are some questions about how strict you want the input validation to be. For instance, in the current PR, minimize_ipopt is a bit more flexible than scipy.optimize.minimize w.r.t. bounds input because it broadcasts compatible x0/bounds shapes rather than raising an error when the lengths don't match. This is what I would have done if writing minimize from scratch, and I believe it's compatible with all cases that minimize would accept, but it's up to you if you want to be more restrictive.

The next commit simply adds a test file from SciPy verbatim, so it probably doesn't need to be reviewed from scratch. The commit after that, https://github.com/mechmotum/cyipopt/pull/207/commits/270af6aa0635068ef234f318cd9d3c16d31def40, makes the (bare minimum, for the most part) adjustments needed to make the tests pass. That would be worth reviewing. The tests caught some minor bugs, so there are a few corrections to minimize_ipopt.

I think that the only other commit that needs to be reviewed is the last one, https://github.com/mechmotum/cyipopt/pull/207/commits/17e8a328e82b709cdddf14e4f7b5c8ba362ec939. Again, the previous commit just adds a test file from SciPy verbatim, so this commit just makes the changes needed to make tests pass.

I see that there is a failure on some jobs, but I can't tell what's wrong with that version of the software. The purpose of the test is to make sure that the correct result is produced when the bounds fully specify the solution. Can we xfail the test in those jobs? xfailed.

Also, would you be willing to set a minimum supported SciPy version? It would be nice to simplify some of the import code, and perhaps we could test against some of the old versions.

mdhaber commented 1 year ago

@moorepants just thought I'd send a ping. I know this looks pretty extensive, but I wrote in https://github.com/mechmotum/cyipopt/pull/207#issuecomment-1566406440 why it shouldn't be that bad to review.

After this, I think I'm done with what I had planned to make minimize_ipopt a nearly drop-in replacement for minimize. There are a few other things that could be done, but I wouldn't add them unless it's requested.

Thanks for your help!

moorepants commented 1 year ago

I have not looked at this and don't recall the status. If it is ready for review I could have a look during August. It looks like a number of CI runs have failed though.

mdhaber commented 1 year ago

I think it's ready for review because I'd like to know how you'd like to handle the failure produced by test_equal_all_bounds. I can't reproduce it locally, even with the same version of SciPy, so I'm not sure what the problem is. Here is a snippet that replicates the test, which I've xfailed in the meantime.

from scipy.optimize import rosen, Bounds
from cyipopt import minimize_ipopt
bounds = Bounds([4.0, 5.0], [4.0, 5.0])
minimize_ipopt(rosen, [-10, 8], bounds=bounds)
# message: b'Algorithm terminated successfully at a locally optimal point, satisfying the convergence tolerances (can be specified by options).'
# success: True
#  status: 0
#     fun: 12109.0
#       x: [ 4.000e+00  5.000e+00]
#     nit: 0
#    info:     status: 0
#                   x: [ 4.000e+00  5.000e+00]
#                   g: []
#             obj_val: 12109.0
#              mult_g: []
#            mult_x_L: [ 1.761e+04  0.000e+00]
#            mult_x_U: [ 0.000e+00  2.200e+03]
#          status_msg: b'Algorithm terminated successfully at a locally optimal point, satisfying the convergence tolerances (can be specified by options).'
#    nfev: 1
#    njev: 3

Can you reproduce the error?

I've pushed a commit that will help me figure out what's going on with the other test. Update: Looks like Ipopt just finds a different but equally good minimizer on some platforms.

moorepants commented 1 year ago

The vast majority of the lines are tests copy-pasted from SciPy.

If you are copying lines from SciPy we need to include SciPy's license in this repo. I'm not sure if we discussed this before, but ideally we don't have to have two licenses in this code base. Are the copy paste lines from SciPy necessary? Can you not import from SciPy instead?

moorepants commented 1 year ago

Also, would you be willing to set a minimum supported SciPy version?

Yes. My general approach has always been to support the SciPy version that shipped with the most recent Ubuntu LTS. It looks like SciPy 1.8.0 was with 22.04 LTS.

moorepants commented 1 year ago

One option for the SciPy license is that we do not ship these two test files with the released tarball. We could include the SciPy license in the test directory for those two SciPy (if you download from this repository) but we specifically do not include the files in the release and thus avoid having to add the SciPy license to the release. If possible, I think that is my preference.

mdhaber commented 1 year ago

Are the copy paste lines from SciPy necessary? Can you not import from SciPy instead?

Yeah they needed slight modification. Pretty much all of those changes are in the commit TST: minimize_ipopt: make new tests pass.

We could include the SciPy license in the test directory for those two SciPy

Good call. The SciPy license is very permissive (New BSD) though. I can note which tests are copied from SciPy and include this license in the source file. Would you like me to do that?

mdhaber commented 1 year ago

Yes. My general approach has always been to support the SciPy version that shipped with the most recent Ubuntu LTS. It looks like SciPy 1.8.0 was with 22.04 LTS.

Great. I think we can make that a separate PR, though.

mdhaber commented 1 year ago

@moorepants I think I've replied to your comments.

It sounds like I need to:

moorepants commented 1 year ago

Add the SciPy license in the test directory. Shall I compbine test_scipy_ipopt_from_scipy.py and test_scipy_ipopt_trust_constr.py and include the license just in that file?

Yes, this sounds good. Then we can exclude that single file in the MANIFEST.in so it doesn't ship in the source tarball.

mdhaber commented 12 months ago

Done. Does GitHub save artifacts from the doc build? I'd like to see how those links rendered.

moorepants commented 12 months ago

Done. Does GitHub save artifacts from the doc build? I'd like to see how those links rendered.

No, you have to build the docs yourself locally.

moorepants commented 12 months ago

I'm good with this and can merge. I'll wait to see if you want to check the docs render.

Thanks for the addition! I'll work on a new release of cyipopt next.

mdhaber commented 12 months ago

The documentation changes here look fine. image

The documentation could probably use another pass at some point, but maybe let's make that a separate PR since it's unrelated. Thanks!

moorepants commented 12 months ago

Sounds good. Merging.