MAINT: minimize_cyipopt: add input validation #206
Conversation
cyipopt/scipy_interface.py
Outdated
| if jac is not None and jac not in {True, False} and not callable(jac): | ||
| raise ValueError('`jac` must be callable or boolean.') | ||
| if hess is not None and not callable(hess): | ||
| raise ValueError('`hess` must be callable.') | ||
| if hessp is not None: | ||
| raise NotImplementedError('`hessp` is not yet supported by Ipopt.`') |
There was a problem hiding this comment.
Previously there was some input validation of jac and hessp in IpoptProblemWrapper, but this will happen first. If IpoptProblemWrapper is only used by minimize_ipopt, shall I remove the (now redundant) input validation of these things there?
There was a problem hiding this comment.
Given that IpoptProblemWrapper is an independent class, any input validation for it belongs there. So my recommendation is to validate everything you can there and then whatever is left could be validated here.
There was a problem hiding this comment.
I actually started there, but then I moved everything out because. It looked like it might just be a support function, and I wouldn't want to break up validation of, say, constraints and bounds if users wouldn't see the difference. Although it doesn't have a preceding underscore, it's not part of __all__ here or in ipopt_wrapper.py, and its not documented online. I couldn't tell - is it considered public?
If you confirm, I'll move what I can.
There was a problem hiding this comment.
I couldn't tell - is it considered public?
Everything is public unless it has been explicitly labeled as not public.
There was a problem hiding this comment.
Ok, I'll move what can be moved.
cyipopt/scipy_interface.py
Outdated
| if not np.iterable(args): | ||
| args = (args,) |
There was a problem hiding this comment.
We've discussed how to handle this situation in SciPy (scipy/scipy#15994), but we haven't really reached a conclusion. This implements one of the suggestions given there. Another possibility is to raise an error (instead of wrapping args in a tuple) if args is not iterable or to be even more strict and require that args is a tuple. LMK which you prefer.
There was a problem hiding this comment.
I think we should copy the behavior in scipy's minimize() which seems to require a tuple: https://docs.scipy.org/doc/scipy/reference/generated/scipy.optimize.minimize.html
There was a problem hiding this comment.
Well, it wraps in a tuple if it's not already one:
https://github.com/scipy/scipy/blob/c1ed5ece8ffbf05356a22a8106affcd11bd3aee0/scipy/optimize/_minimize.py#L527
Ok, I'll do that.
There was a problem hiding this comment.
| if not np.iterable(args): | |
| args = (args,) | |
| if not isinstance(args, tuple): | |
| args = (args,) |
| kwargs = dict() if kwargs is None else kwargs | ||
| if not isinstance(kwargs, dict): | ||
| raise ValueError('`kwargs` must be a dictionary.') | ||
| if method is not None: # this will be updated when gh-200 is merged |
There was a problem hiding this comment.
This and gh-200 can be merged in either order. I'll make sure the other gets updated accordingly.
cyipopt/scipy_interface.py
Outdated
| # TODO: add input validation for `bounds` and `constraints` when adding | ||
| # support for instances of new-style constraints (e.g. `Bounds` and | ||
| # `NonlinearConstraint`) and sequences of constraints. |
There was a problem hiding this comment.
This is next on my list.
mdhaber
force-pushed
the
add_input_validation
branch
from
b256d20 to
ddec62d
Compare
|
LGTM! Thanks a bunch. I like your test strings :) |
| @@ -118,8 +135,7 @@ def __init__(self, | |||
| elif jac is True: | |||
| fun = MemoizeJac(fun) | |||
| jac = fun.derivative | |||
| elif not callable(jac): | |||
| raise NotImplementedError('jac has to be bool or a function') | |||
There was a problem hiding this comment.
I should have pointed out that I changed this to a ValueError above because I'm not sure if there is anything specific to be implemented. (hessp is different because there is something specific we have in mind.) Hope that's OK. If not, I can change it back in the next PR (in which I'll add support for Bounds).
There was a problem hiding this comment.
I think it would be a rare case that someone relied on this as a feature, so we can go with your change.
Although
IpoptProblemWrappercontains some input validation,minimize_ipoptwas missing input validation of other parameters. For instance,callbackwas ignored silently, and invalid values of arguments could lead to confusing errors deeper in the code. This PR systematically adds basic input validation and tests tominimize_ipopt.