-
Notifications
You must be signed in to change notification settings - Fork 56
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: allow for print_level greater than 1 and ENH: allow warm start for dual variables #239
Conversation
Can you please add unit tests for these changes? Thanks for the contribution. |
Sorry, I don't quite know how to add unit test. I just tested them with my own codes. Should I add some test script or something? |
We try to add a unit test, for example see https://github.com/mechmotum/cyipopt/blob/master/cyipopt/tests/unit/test_ipopt_funcs.py, to test any new additions/changes. |
cyipopt/scipy_interface.py
Outdated
callback=None, | ||
options=None): | ||
options=None, | ||
mult_g=[], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that using mutable default arguments in function signatures can cause bugs if the function is called more than once. See the "Mutable Default Arguments" section of this article for an explanation and suggestion on how to avoid. A simple way to avoid this foot gun is to use None
as the default arguments in the function signature and then manually create a new instance of an empty list within the function body if the value of the variable in question is None
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your patience! I did neglect this.
if getattr(options, 'print_level', False) is True: | ||
options[b'print_level'] = 1 | ||
if b'print_level' in options: | ||
if options[b'print_level'] is True: | ||
options[b'print_level'] = 1 | ||
elif options[b'print_level'] is False: | ||
options[b'print_level'] = 0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change doesn't appear to make any difference to the way the code functions, except increasing the number of lines of code to do the same thing. Am I missing something? If so, please can you explain why this change is needed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ipopt print levels are integers between 1 and 12, with the default being 5: https://coin-or.github.io/Ipopt/OPTIONS.html#OPT_print_level
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should use something like the following. This will ensure that if a print_level
of True
is used that the default print level is used, not breaking previous code. It will also allow users to manually select the exact desired print level as per the Ipopt options.
if getattr(options, 'print_level', False) is True: | |
options[b'print_level'] = 1 | |
if b'print_level' in options: | |
if options[b'print_level'] is True: | |
options[b'print_level'] = 1 | |
elif options[b'print_level'] is False: | |
options[b'print_level'] = 0 | |
if getattr(options, 'print_level') is True: | |
options[b'print_level'] = 5 | |
else: | |
print_level = getattr(options, 'print_level' , 5) | |
if not isinstance(print_level, int) or (print_level < 1) or (print_level > 12): | |
raise ValueError(f'`print_level` option must be an int between 1 and 12, not {print_level}') | |
options[b'print_level'] = print_level | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought it this way: The original version would force the print_level
to be 1
if the user has specified this parameter, no matter what value is chosen. I tried to make it work like this:
- 'disp':
True
-> 'print_level':1
- 'disp':
False
-> 'print_level':0
- no 'disp' or 'print_level' -> 'print_level':
0
(according to Ipopt's document, aprint_level
of value0
is acceptable) - 'disp'/'print_level':
0
-12
-> 'print_level':0
-12
About the default print_level
in the second case, I did have some doubts. I wondered before why Ipopt said that print_level
was by default 5
, but that was not how it actually behavioured. So when I was changing these codes, I hesitated about whether to set the default value to 1
or 5
. I chose 1
eventually because it was the original setting.
That's weird. The checks seemed to fail because of the missing of SciPy. I don't understand what's going on. I've run |
Please merge in the tip of master to get the latest fixes for some scipy tests. |
It seems that all the tests I added in |
We use the pytest skip decorator to skip the tests if scipy is not installed: |
Yeah I know. I mean, since all the tests I added are based on SciPy, skipping the scipy tests would make all these tests meaningless. |
They are not meaningless because we run them all here in the CI system and anyone that has scipy installed and runs the tests will also run them. |
Got it. Thanks for explanation. |
|
||
def _dual_initial_guess_validation(dual_name: str, dual_value: list, length: int): | ||
if not isinstance(dual_value, list): | ||
raise TypeError(f'`{dual_name}` must be a list.') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why does this have to be a list? Couldn't any iterable or sequence work?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since Problem.solve()
uses empty lists as the default values for lagrange
, zl
and zu
, and create zero numpy arrays when they are []
, I just followed it. Indeed any iterable or sequence would work.
cyipopt/cyipopt/cython/ipopt_wrapper.pyx
Line 628 in 2048826
if lagrange == []: |
options=None, | ||
mult_g=None, | ||
mult_x_L=None, | ||
mult_x_U=None,): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This function is supposed to mirror the scipy minimize()
function. The new arguments you have added are not available in SciPy's minimize()
function. Shouldn't then these new arguments be passed in through the options
keyword which allows you to provide options for a specific solver?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see. But I thought all keys in options
are or could be transferred into options supported by ipopt (am I wrong?), wouldn't it be missleading if these three arguments are passed through options
?
fix #234 and #151