mechmotum / cyipopt

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

Fix #234 #244

Closed MarkusZimmerDLR closed 9 months ago

MarkusZimmerDLR commented 9 months ago

Hotfix for print_level #234 to allow integer other than 0 or 1. Bools are also converted this way.

Commit mistakenly references #239. No idea how to fix this after the fact.

moorepants commented 9 months ago

This works for options={b'print_level': INTEGER} or options={'disp': INTEGER} but fails for options={b'print_level': True/False}. Do you intend for it to work with the boolean? Did it before?

MarkusZimmerDLR commented 9 months ago

how does it fail? A bool in python is nothing but an integer. int(True) = 1 And the previous line did nothing but replacing True with 1 and False with 0

moorepants commented 9 months ago
===============================================================================
Traceback (most recent call last):
  File "/home/moorepants/miniconda/envs/cyipopt-dev/lib/python3.11/site-packages/cyipopt/scipy_interface.py", line 587, in minimize_ipopt
    nlp.add_option(option, value)
  File "cyipopt/cython/ipopt_wrapper.pyx", line 503, in ipopt_wrapper.Problem.add_option
    raise TypeError("Invalid option type")
TypeError: Invalid option type

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "/home/moorepants/src/cyipopt/examples/rosen.py", line 18, in <module>
    res = minimize_ipopt(rosen, x0, jac=rosen_der, options= {b'print_level': True})
          ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/moorepants/miniconda/envs/cyipopt-dev/lib/python3.11/site-packages/cyipopt/scipy_interface.py", line 590, in minimize_ipopt
    raise TypeError(msg.format(option, value, e))
TypeError: Invalid option for IPOPT: b'print_level': True (Original message: "Invalid option type")
moorepants commented 9 months ago

Same with disp:

Traceback (most recent call last):
  File "/home/moorepants/miniconda/envs/cyipopt-dev/lib/python3.11/site-packages/cyipopt/scipy_interface.py", line 587, in minimize_ipopt
    nlp.add_option(option, value)
  File "cyipopt/cython/ipopt_wrapper.pyx", line 503, in ipopt_wrapper.Problem.add_option
    raise TypeError("Invalid option type")
TypeError: Invalid option type

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "/home/moorepants/src/cyipopt/examples/rosen.py", line 18, in <module>
    res = minimize_ipopt(rosen, x0, jac=rosen_der, options= {'disp': True})
          ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/moorepants/miniconda/envs/cyipopt-dev/lib/python3.11/site-packages/cyipopt/scipy_interface.py", line 590, in minimize_ipopt
    raise TypeError(msg.format(option, value, e))
TypeError: Invalid option for IPOPT: b'print_level': True (Original message: "Invalid option type")
moorepants commented 9 months ago

Probably best to add a unit test that at least ensures these errors don't occur (you don't have to check whether the correct things are shown on stdout).

moorepants commented 9 months ago

Also, the default for minimize_ipopt should show the least amount of info from IPOPT (ideally no info).

MarkusZimmerDLR commented 9 months ago

The default of 5 is taken from the official ipopt site https://coin-or.github.io/Ipopt/OPTIONS.html#OPT_print_level

I tested the code on my side. And the results are correct.

>>> options = {'print_level' : True}
>>> convert_to_bytes(options)
>>> replace_option(options, b'disp',b'print_level')
>>> int(options.get(b'print_level', 5))
1
>>> options = {'disp' : True}
>>> convert_to_bytes(options)
>>> replace_option(options, b'disp',b'print_level')
>>> int(options.get(b'print_level', 5))
1
>>> options = {b'print_level' : True}
>>> convert_to_bytes(options)
>>> replace_option(options, b'disp',b'print_level')
>>> int(options.get(b'print_level', 5))
1
>>> options = {b'disp' : True}
>>> convert_to_bytes(options)
>>> replace_option(options, b'disp',b'print_level')
>>> int(options.get(b'print_level', 5))
1
moorepants commented 9 months ago

The default of 5 is taken from the official ipopt site https://coin-or.github.io/Ipopt/OPTIONS.html#OPT_print_level

That's fine, but that is 1) not what we currently have and 2) not what is desired for the minimize_ipopt() function. This function should not display any output unless requested, just like SciPy's minimize() function.

I tested the code on my side. And the results are correct.

Did you test with the minimize_ipopt() function? I pasted the errors I'm getting above.

moorepants commented 9 months ago

Did you test with the minimize_ipopt() function? I pasted the errors I'm getting above.

I may be picking up the old code, double checking.

MarkusZimmerDLR commented 9 months ago

cloned my PR, ran pytest:

==================== 98 passed, 7 skipped, 1 xpassed, 46 warnings in 8.17s ====================
moorepants commented 9 months ago

Ok, I rechecked an True/False work. I was picking up the wrong cyipopt install or something.

If you change the default to display no output, I'll merge. Thanks.

moorepants commented 9 months ago

Great. Thanks for the fix.