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

Allow callables as optimizers in VQE and QAOA #7191

Merged
merged 17 commits into from
Mar 15, 2022

Conversation

Cryoris
Copy link
Contributor

@Cryoris Cryoris commented Oct 29, 2021

Summary

Closes #6383.

Details and comments

Allow callables with the following signature as optimizers in the VQE and QAOA:

def my_optimizer(fun, x0, jac=None, bounds=None):

where the result can be either of qiskit.algorithms.optimizers.OptimizerResult or scipy.optimize.OptimizeResult.
This also allows to directly pass SciPy's optimizers into these algorithms, e.g. as

optimizer = partial(minimize, method="L-BFGS-B", options={"maxiter": 100})

@Cryoris Cryoris requested review from manoelmarques, woodsp-ibm and a team as code owners October 29, 2021 08:53
@Cryoris Cryoris added this to the 0.19 milestone Oct 29, 2021
@Cryoris Cryoris added Changelog: New Feature Include in the "Added" section of the changelog mod: algorithms Related to the Algorithms module labels Oct 29, 2021
@coveralls
Copy link

coveralls commented Nov 10, 2021

Pull Request Test Coverage Report for Build 1986651072

  • 17 of 20 (85.0%) changed or added relevant lines in 2 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.003%) to 83.465%

Changes Missing Coverage Covered Lines Changed/Added Lines %
qiskit/algorithms/minimum_eigen_solvers/vqe.py 16 19 84.21%
Totals Coverage Status
Change from base Build 1983451324: 0.003%
Covered Lines: 52342
Relevant Lines: 62711

💛 - Coveralls

@Cryoris Cryoris added the on hold Can not fix yet label Nov 11, 2021
@Cryoris Cryoris modified the milestones: 0.19, 0.20 Nov 11, 2021
@Cryoris
Copy link
Contributor Author

Cryoris commented Nov 11, 2021

Marking this as on hold until Python 3.6 support is dropped (which is the next release) so we can import Protocol from typing and don't need to add a new dependency to typing_extensions.

@jakelishman
Copy link
Member

I think typing.Protocol only turned up in Python 3.8, unfortunately. You could potentially work around it by making OptimizerHandle.__call__ an abstract method with a particular type hint.

@Cryoris
Copy link
Contributor Author

Cryoris commented Nov 12, 2021

Yep the documentation says you're right... How would the abstract variant look like? Otherwise we can maybe just add the additional requirement 😄

@jakelishman
Copy link
Member

As long as you're happy having everything derive from OptimizerHandle as its second class, then you don't need typing.Protocol because the type-checker will be automatically satisfied. typing.Protocol is for cases where you want things to work without having them derive from the interfaces - in this case, that's likely def'd functions, which can never pass the ABC type-checking. It's not ideal, but you can get the library components to pass type checking for now with

class OptimizerHandle(abc.ABC):
    @abc.abstractmethod
    def __call__(self, ...) -> ...: ...

and just continue to have the classes you want derive from it.

@Cryoris
Copy link
Contributor Author

Cryoris commented Nov 15, 2021

The goal is to pass in functions from SciPy and user-defined ones, so without inheriting from an interface. The other solution, if we want to avoid typing_extensions would just be a tediously long standard type hint.

@jakelishman
Copy link
Member

jakelishman commented Nov 15, 2021

I mean, you can technically fulfill the type hinting by just setting it to Callable, or just accept that it won't type check completely anyway - mypy currently crashes if you actually try to run it on Terra, haha. You could also define an alias type and re-use that, though it doesn't help in the Sphinx documentation because it gets expanded. For example, quantumcircuit.py does

QubitSpecifier = Union[
    Qubit,
    QuantumRegister,
    int,
    slice,
    Sequence[Union[Qubit, int]],
]

and re-uses that in places.

@Cryoris
Copy link
Contributor Author

Cryoris commented Nov 15, 2021

Hmm yeah but at least it'll make the code more readable and users should get from the docs what function they can pass in. Let's go with the long type hint then!

@Cryoris Cryoris removed the on hold Can not fix yet label Dec 10, 2021
@manoelmarques
Copy link
Member

I know all the code is in VQE but maybe adding a unit test to test_qaoa.py too ? Otherwise it looks good to me.

manoelmarques
manoelmarques previously approved these changes Dec 10, 2021
Copy link
Member

@jakelishman jakelishman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd agree that you should probably duplicate your tests into QAOA as well, just to be sure.

Comment on lines +52 to +66
OBJECTIVE = Callable[[np.ndarray], float]
GRADIENT = Callable[[np.ndarray], np.ndarray]
RESULT = Union[scipy.optimize.OptimizeResult, OptimizerResult]
BOUNDS = List[Tuple[float, float]]

MINIMIZER = Callable[
[
OBJECTIVE, # the objective function to minimize (the energy in the case of the VQE)
np.ndarray, # the initial point for the optimization
Optional[GRADIENT], # the gradient of the objective function
Optional[BOUNDS], # parameters bounds for the optimization
],
RESULT, # a result object (either SciPy's or Qiskit's)
]

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this type hint actually correct for any SciPy minimiser? It seems to imply that the first four arguments are positional, but the first four positional arguments of their minimisers are (objective, x0, args, jac).

In an ideal world, I think we'd put this in __init__.py, and write some documentation in the docstring about the types. It can have a Sphinx cross-ref (.. _algorithms_minimum_eigensolvers_minimizer:, then you reference with :ref:algorithms_minimum_eigensolvers_minimizer`), then the various class docstrings could all link to it. That said, I don't think you need to worry much about it right now.

Copy link
Contributor Author

@Cryoris Cryoris Feb 23, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In fact the callable must have the named arguments fun, x0, jac, bounds but I didn't quite know how to put that in a type hint using only what's available now 🤔 I updated the class docs to highlight this

Copy link
Contributor

@ikkoham ikkoham Feb 25, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how about using Callback Protocol? PEP544

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We tried that but it's only supported from Python 3.8 onwards, see the discussion above: #7191 (comment)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Protocol is back ported by typing-extensions. https://pypi.org/project/typing-extensions/ so you can use it.
(I'm sorry I overlooked...)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah but that would create an additional dependency to a new package, so we didn't add it (yet) 😄

qiskit/algorithms/minimum_eigen_solvers/vqe.py Outdated Show resolved Hide resolved
qiskit/algorithms/minimum_eigen_solvers/vqe.py Outdated Show resolved Hide resolved
Comment on lines +19 to +21
result = OptimizerResult()
result.x = # optimal parameters
result.fun = # optimal function value
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Kind of weird that you don't construct the whole class in one go at initialisation time, but I gave up that fight in Python long ago.

test/python/algorithms/test_vqe.py Outdated Show resolved Hide resolved
@ikkoham
Copy link
Contributor

ikkoham commented Dec 13, 2021

spicy.optimize.minimize https://docs.scipy.org/doc/scipy/reference/generated/scipy.optimize.minimize.html can take a callable as a method. ScipyOptimizer is already designed to take a Callable, but is there a use case where this is not sufficient?

@woodsp-ibm
Copy link
Member

@Cryoris Where are we with this. I looked it up since I recalled this level of support but see its not there yet. In the external Slack applications channel someone was asking about passing other arguments into BOBYQA that internally on the skquant minimize is done via kwargs but that cannot be passed into the BOBYQA "wrapper" we have here. This would have allowed them to do it using the skquant minimize more directly. For ref the thread is here https://qiskit.slack.com/archives/CB6C24TPB/p1645196828099929

@Cryoris
Copy link
Contributor Author

Cryoris commented Feb 23, 2022

spicy.optimize.minimize https://docs.scipy.org/doc/scipy/reference/generated/scipy.optimize.minimize.html can take a callable as a method. ScipyOptimizer is already designed to take a Callable, but is there a use case where this is not sufficient?

Yes @ikkoham, it's principally possible to go through the SciPyOptimizer by doing

from qiskit.algorithms.optimizers import SciPyOptimizer

def my_optimizer(fun, x0, args, **kwargs):
    return # some scipy.optimize.OptimizeResult()

optimizer = SciPyOptimizer(method=my_optimizer)
vqe = VQE(optimizer=optimizer)

However with this PR we'd simplify this process by not having to go through this extra object. This PR would probably make using the SciPyOptimizer redundant since once can just do

from functools import partial
from scipy import minimize

vqe = VQE(optimizer=partial(minimize, method="L-BFGS-B or your method of choice"))

This PR is part of the redesign accoring to the design doc we discussed a while back, you can check it here: #6381. If you any concerns with using callables directly I'm happy to discuss it!

@ikkoham
Copy link
Contributor

ikkoham commented Feb 25, 2022

@Cryoris Thanks. I understand why you want to add this and I agree with you. This is useful and right direction.

@Cryoris Cryoris requested a review from manoelmarques March 9, 2022 11:56
manoelmarques
manoelmarques previously approved these changes Mar 9, 2022
ikkoham
ikkoham previously approved these changes Mar 10, 2022
Copy link
Contributor

@ikkoham ikkoham left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK. I can update Callback Protocol after merged.

@ikkoham ikkoham dismissed stale reviews from manoelmarques and themself via e429f5c March 10, 2022 15:43
Copy link
Contributor

@ikkoham ikkoham left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I resolved the merge conflict. Approve again.

@mergify mergify bot merged commit 4519865 into Qiskit:main Mar 15, 2022
@Cryoris Cryoris deleted the vqe-callables branch March 15, 2022 20:34
ElePT pushed a commit to ElePT/qiskit that referenced this pull request Jun 27, 2023
* allow callables as optimizers in VQE

* Missing quote

* fix lint

* don't use typing.Protocol

* comments from review

* add qaoa test

* lint

Co-authored-by: Steve Wood <40241007+woodsp-ibm@users.noreply.github.com>
Co-authored-by: Ikko Hamamura <ikkoham@users.noreply.github.com>
Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
ElePT pushed a commit to ElePT/qiskit-algorithms-test that referenced this pull request Jul 17, 2023
* allow callables as optimizers in VQE

* Missing quote

* fix lint

* don't use typing.Protocol

* comments from review

* add qaoa test

* lint

Co-authored-by: Steve Wood <40241007+woodsp-ibm@users.noreply.github.com>
Co-authored-by: Ikko Hamamura <ikkoham@users.noreply.github.com>
Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Changelog: New Feature Include in the "Added" section of the changelog mod: algorithms Related to the Algorithms module
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow callables as optimizers in VQE
6 participants