-
Notifications
You must be signed in to change notification settings - Fork 65
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 bounds parsing in Scipy optimizers and warn when unsupported #155
Conversation
This will need a test case that used to fail that does not know so it can be tested around things now work as expected and continue to do so. And if (see discussion below) it raises a UserWarning a test to check that happens as expected. Discussion: As the optimizers were historically they called scipy.optimize.minimize - well they still do for the scipy ones. To have these as objects that could be created and passed, rather than have to deal with functions and partials, that would not have fitted with the design when these were first created, values that needed to end up in the So to me bounds ought not to be passed in options as that should strictly just be the scipy minimize options. I think options was exposed here as not all possible options had been exposed as parameters, since initially we made a choice on the ones we expected to be used for the problems that were then being addressed. Having bounds as kwargs in the init seems ok, then does one choose to enforce that as the bounds if an algorithm is setting it via some other mechanism, like VQE does, when it calls minimize. Or let bounds on the minimize override that a user might pass in via kwargs - or have another parameter that you can set to say which overrides which. Last I am not sure I would want to check if bounds is supported and raise a user error (you don;t check other stuff like jac and it seems like if we did it would be a rabbit hole to get lost in). This scipy base class was designed to be more flexible and allow you to pass the Dealing with the bounds need some thought/discussion - hence the issue raised i.e. #57. Maybe a simple change could tide things over for now and allow something. @Cryoris You were more involved in this ScipyOptimizer than I, any thoughts? |
Thanks, @woodsp-ibm, I'll work on the two tests you suggested. |
Pull Request Test Coverage Report for Build 8804213726Details
💛 - Coveralls |
In the fork, |
I agree with @woodsp-ibm above, we should follow SciPy's interface as closely as possible, which includes the same usage of |
I have implemented the suggested changes and adapted the bounds parsing to be as close as possible to the Scipy usage. As in Scipy, bounds can only be parsed in the |
Co-authored-by: Steve Wood <40241007+woodsp-ibm@users.noreply.github.com>
Co-authored-by: Steve Wood <40241007+woodsp-ibm@users.noreply.github.com>
Co-authored-by: Steve Wood <40241007+woodsp-ibm@users.noreply.github.com>
Co-authored-by: Steve Wood <40241007+woodsp-ibm@users.noreply.github.com>
# Conflicts: # qiskit_algorithms/optimizers/scipy_optimizer.py # test/optimizers/test_optimizers.py
I have applied the suggested changes and the PR is now ready. What's your feedback, and what are the next steps? |
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.
Options was always meant to mean options as per the scipy optimizer in question. I guess if people are trying to get bounds in through there - maybe the options docstring could be clearer. Do we want to detect/add a warning since its a common error - is it that common? If you did that directly to scipy and added it in the options that is how it would fail. I guess you are unlikely to do that since its mnimize exposes the bounds same as we do here. It just that when constructing one of these that no bound can be specified at that point and bounds comes down to be supplied by whatever algo is calling minimize however it gets bounds.
I will not that the base class has a set_options so even with the check here, done in constructor, if it were set that way it would fail since options is only checked when first passed there and not if updated.
Whatever this nets out as, since its a change that affects behavior from a end user perspective - well at least in terms of the error they get is more meaningful, hopefully, this should have a release note too I think. |
Co-authored-by: Steve Wood <40241007+woodsp-ibm@users.noreply.github.com>
I agree that |
…kit-community#155) Co-authored-by: Steve Wood <40241007+woodsp-ibm@users.noreply.github.com>
Summary
Fixes the parsing of bounds to the Scipy optimizers, which previously raised the error below.
The present PR fixes this behaviour documented in qiskit-community/qiskit-machine-learning#570 and further discussed in #57.
Details and comments
This PR contains two updates. The first one gathers the bounds from
_options
or_kwargs
if present, then copies them ontbounds
, finally it deletes the key in the dictionaries, avoiding clashes in the https://github.com/edoaltamura/qiskit-algorithms/blob/77abbcbbeedba111cfde75df0cfea2940c9790dc/qiskit_algorithms/optimizers/scipy_optimizer.py#L165. The changes are below:https://github.com/edoaltamura/qiskit-algorithms/blob/77abbcbbeedba111cfde75df0cfea2940c9790dc/qiskit_algorithms/optimizers/scipy_optimizer.py#L121-L127
The second update returns a warning when trying to assign a non-None bound value with an optimizer method that doesn't support it. The class
QiskitAlgorithmsOptimizersWarning
is defined in:https://github.com/edoaltamura/qiskit-algorithms/blob/77abbcbbeedba111cfde75df0cfea2940c9790dc/qiskit_algorithms/exceptions.py#L24
For the bounds parsing, the derived class
QiskitAlgorithmsWarning
is defined and used:https://github.com/edoaltamura/qiskit-algorithms/blob/77abbcbbeedba111cfde75df0cfea2940c9790dc/qiskit_algorithms/exceptions.py#L36
✅ I have added the tests to cover my changes.
✅ I have updated the documentation accordingly.
✅ I have read the CONTRIBUTING document.