Skip to content
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

Fix #234 #244

Merged
merged 2 commits into from
Nov 29, 2023
Merged

Fix #234 #244

merged 2 commits into from
Nov 29, 2023

Conversation

MarkusZimmerDLR
Copy link
Contributor

@MarkusZimmerDLR MarkusZimmerDLR commented Nov 29, 2023

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.

@MarkusZimmerDLR MarkusZimmerDLR changed the title Fix #239 Fix #234 Nov 29, 2023
@moorepants
Copy link
Collaborator

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
Copy link
Contributor Author

MarkusZimmerDLR commented Nov 29, 2023

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
Copy link
Collaborator

===============================================================================
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
Copy link
Collaborator

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
Copy link
Collaborator

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
Copy link
Collaborator

moorepants commented Nov 29, 2023

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

@MarkusZimmerDLR
Copy link
Contributor Author

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
Copy link
Collaborator

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
Copy link
Collaborator

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
Copy link
Contributor Author

cloned my PR, ran pytest:

==================== 98 passed, 7 skipped, 1 xpassed, 46 warnings in 8.17s ====================

@moorepants
Copy link
Collaborator

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
Copy link
Collaborator

Great. Thanks for the fix.

@moorepants moorepants merged commit 0b12acc into mechmotum:master Nov 29, 2023
@MarkusZimmerDLR MarkusZimmerDLR deleted the patch-1 branch November 29, 2023 14:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants