-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
VQE implementation with estimator primitive #8702
VQE implementation with estimator primitive #8702
Conversation
Thank you for opening a new pull request. Before your PR can be merged it will first need to pass continuous integration tests and be reviewed. Sometimes the review process can be slow, so please be patient. While you're waiting, please feel free to review other open PRs. While only a subset of people are authorized to approve pull requests for merging, everyone is encouraged to review open pull requests. Doing reviews helps reduce the burden on the core team and helps make the project's code better for everyone. One or more of the the following people are requested to review this:
|
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.
The overall structure LGTM! I left some
brief comments below 🙂
qiskit/algorithms/minimum_eigensolvers/numpy_minimum_eigensolver.py
Outdated
Show resolved
Hide resolved
Pull Request Test Coverage Report for Build 3135497477
💛 - Coveralls |
1c683e1
to
7d05317
Compare
qiskit/algorithms/minimum_eigensolvers/numpy_minimum_eigensolver.py
Outdated
Show resolved
Hide resolved
qiskit/algorithms/minimum_eigensolvers/numpy_minimum_eigensolver.py
Outdated
Show resolved
Hide resolved
test/python/algorithms/minimum_eigensolvers/test_numpy_minimum_eigensolver.py
Outdated
Show resolved
Hide resolved
test/python/algorithms/minimum_eigensolvers/test_numpy_minimum_eigensolver.py
Outdated
Show resolved
Hide resolved
qiskit/algorithms/minimum_eigensolvers/numpy_minimum_eigensolver.py
Outdated
Show resolved
Hide resolved
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.
Just some minor comments/questions from my side 👍
1795747
to
d60b231
Compare
I note that
|
It seems sensible/consistent to have access to other information for the main operator too. I had suggested in the estimate_observables passing back the whole metadata #8683 (comment) while perhaps keeping the extracted variance as a first class field. #8105 may change things up. |
I think that adding the About #8105, it looks like the discussion hasn't progressee in a while, so it might change things in the future, but I don't think we should worry about it for this release. |
Yes, there the variance is already taken out of the metadata - the resultant tuple of (variance, shots) is always present with either defaulting to 0 if its not in the metadata. All I meant by first class field was extracting it out of the metadata like the estimate observables does. It would be nice to see some of what is in the metdata more directly as fields estimator result etc you can know what is in the result, IDE auto-completion and so on. Having it "hidden" in metadata seems less ideal which is an aspects #8105 was seeking to address too I think. |
6ecf813
to
ffc67c7
Compare
55e6ca6
to
b1c55ca
Compare
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 so much Declan and Steve for all the effort, especially over the past day. My approval is not very powerful, but I believe all the comments have been addressed and it looks good to me. If @t-imamichi or @ikkoham could approve it, it would unblock our workflow for the PRs that depend on VQE :)
@@ -168,6 +168,7 @@ def evolve(self, evolution_problem: TimeEvolutionProblem) -> TimeEvolutionResult | |||
self.estimator, | |||
evolved_state, | |||
evolution_problem.aux_operators, | |||
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.
Is it related to VQE?
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.
It's related to the compute_eigenvalues
method that is also used in VQE. We changed the signature to take optional parameters instead of bound circuits so that the estimator cache does not think it's a different circuit every time (for efficiency). In trotter QRTE the circuits are not parametrized so we had to add this None
in the parameters field.
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.
It would have been possible too to change some of the parameters in the call to using keywords, particularly threshold at the end, That way the None for parameters could have been left as the default value - the call as it stood would have to have been changed either way.
Apply suggestion Imamichi-san
self, | ||
filter_criterion: Callable[ | ||
[list | np.ndarray, float, ListOrDict[float] | None], bool | ||
] = 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.
I'm not sure whether it requires | None
?
] = None, | |
] | None = 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.
The prior signature was
filter_criterion: Callable[[Union[List, np.ndarray], float, Optional[ListOrDict[float]]], bool] = None,
I imagine this is saying that the filter criteria is applied to the aux ops result which may be None. I would have thought the filter would not have needed to be called in this case but this is how it has been.
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.
Change made in the commit that addresses your comment below.
qiskit/algorithms/minimum_eigensolvers/numpy_minimum_eigensolver.py
Outdated
Show resolved
Hide resolved
It looks good overall though I wrote some minor comments. |
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.
Great! lgtm. I left some minor comments.
Co-authored-by: Ikko Hamamura <ikkoham@users.noreply.github.com>
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.
@declanmillar @ElePT Thanks for the effort to get this done!
* Add minimally working VQE with estimator primitive implementation. No gradients, no tests etc. * Revert from dataclass results to original result classes. * Enforce positional and keyword VQE arguments. * Move aux op eval logic to function * Update docstring. * Remove max_evals_grouped. Force to set directly on optimizer. * Remove validate min import. * Make note that eval_observables will be used to eval aux ops. * Add initial vqe tests. * Have VQE inherit from VariationalAlgorithm. * Move energy evaluation to unnested function. * Construct h2_op using SparsePauliOp * Add gradient with primitives support. * Update docstrings * update broadcast handling * update eval_energy output for batching * add incomplete QNSPA test * fix batch evaluation of QNSPSA test * remove vqe callback * move estimator to first arg * remove usused imports * add minimum eigensolvers test init file * add aux ops tests and prepare for new eval_operators * no longer support account for Nones in aux_ops * correct typing for MinimumEigensolver * Compute default initial point using ansatz bounds. * Add NumPyMinimumEigensolverResult * Fix type hints * Fix type hints * Formatting * Do not store NumPyMES result inside the algo. * Provide default values for ansatz and estimator * Formatting * fix old and new batching * Add tests for NumpyMES and import in module. * Use lazy formatting in log messages * Use lazy formatting in log messages * Add back callback to VQE. * minor renaming * raise algorithm error if primitive jobs fail * Add return documentation * Improve var names and docstrings. * Apply suggestions from code review Co-authored-by: ElePT <57907331+ElePT@users.noreply.github.com> Co-authored-by: dlasecki <dal@zurich.ibm.com> * minor formatting * minor formatting * Ensure evaluate_energy/gradient match Co-authored-by: Max Rossmannek <max.rossmannek@uzh.ch> * return bounds logic; fix some docstrings and minor refactor * Force keyword arguments in vqe. * Use estimate_observables function * break up numpy mes tests with subTest * formatting * remove redundant eval_aux_ops * add typehints to docstring attributes * remove usused imports * remove default ansatz * remove usused imports * update typehints * avoid changing the original ansatz * avoid changing the original ansatz * create separate function to build vqe result * Correct aux operator eignvalue type hint Co-authored-by: ElePT <57907331+ElePT@users.noreply.github.com> * add variance to callback * use std_dev in callback rather than variance * formatting * return variance and shots in callback * return full metadata in callback * Move validation functions to algorithms/utils * correct the callback attribute documentation * correct the callback attribute typehint docstring * update VQE docstring * release note and pending-depreciate old algs * update vqe class docstring * Apply suggestions from code review Co-authored-by: ElePT <57907331+ElePT@users.noreply.github.com> * Do not copy ansatz * Note pending depreciation of old algs * fix docstrings and imports * Fix issues with building docs * Include OptimizerResult in VQEResult * Remove trailing whitespace * Fix math notation in docstring * estimate_obervables to return metadata @ElePT +VQE * add example in release note * Update evaluate_observables docstring * Fix observables_evaluator tests. Co-authored-by: ElePT <57907331+ElePT@users.noreply.github.com> * fix trotter_qrte tests and remove depreciation * formatting * remove unused import * Apply suggestions to docstring from code review Co-authored-by: Steve Wood <40241007+woodsp-ibm@users.noreply.github.com> * Update VQE docstring * Remove printing Co-authored-by: ElePT <57907331+ElePT@users.noreply.github.com> * Add parameters to estimate observables * Fix estimate obs. unit test * Update arg description * Update arg description * keep equation part of sentence * dict -> dict[str, Any] * Update qiskit/algorithms/optimizers/spsa.py Apply suggestion Imamichi-san * introduce FilterType and aux_operator_eigenvalues -> aux_operators_evaluated * Correct typehint Co-authored-by: Ikko Hamamura <ikkoham@users.noreply.github.com> * update vqe docstring and use old typing for type alias Co-authored-by: ElePT <57907331+ElePT@users.noreply.github.com> Co-authored-by: dlasecki <dal@zurich.ibm.com> Co-authored-by: Max Rossmannek <max.rossmannek@uzh.ch> Co-authored-by: Steve Wood <40241007+woodsp-ibm@users.noreply.github.com> Co-authored-by: ElePT <epenatap@gmail.com> Co-authored-by: Ikko Hamamura <ikkoham@users.noreply.github.com>
* Add minimally working VQE with estimator primitive implementation. No gradients, no tests etc. * Revert from dataclass results to original result classes. * Enforce positional and keyword VQE arguments. * Move aux op eval logic to function * Update docstring. * Remove max_evals_grouped. Force to set directly on optimizer. * Remove validate min import. * Make note that eval_observables will be used to eval aux ops. * Add initial vqe tests. * Have VQE inherit from VariationalAlgorithm. * Move energy evaluation to unnested function. * Construct h2_op using SparsePauliOp * Add gradient with primitives support. * Update docstrings * update broadcast handling * update eval_energy output for batching * add incomplete QNSPA test * fix batch evaluation of QNSPSA test * remove vqe callback * move estimator to first arg * remove usused imports * add minimum eigensolvers test init file * add aux ops tests and prepare for new eval_operators * no longer support account for Nones in aux_ops * correct typing for MinimumEigensolver * Compute default initial point using ansatz bounds. * Add NumPyMinimumEigensolverResult * Fix type hints * Fix type hints * Formatting * Do not store NumPyMES result inside the algo. * Provide default values for ansatz and estimator * Formatting * fix old and new batching * Add tests for NumpyMES and import in module. * Use lazy formatting in log messages * Use lazy formatting in log messages * Add back callback to VQE. * minor renaming * raise algorithm error if primitive jobs fail * Add return documentation * Improve var names and docstrings. * Apply suggestions from code review Co-authored-by: ElePT <57907331+ElePT@users.noreply.github.com> Co-authored-by: dlasecki <dal@zurich.ibm.com> * minor formatting * minor formatting * Ensure evaluate_energy/gradient match Co-authored-by: Max Rossmannek <max.rossmannek@uzh.ch> * return bounds logic; fix some docstrings and minor refactor * Force keyword arguments in vqe. * Use estimate_observables function * break up numpy mes tests with subTest * formatting * remove redundant eval_aux_ops * add typehints to docstring attributes * remove usused imports * remove default ansatz * remove usused imports * update typehints * avoid changing the original ansatz * avoid changing the original ansatz * create separate function to build vqe result * Correct aux operator eignvalue type hint Co-authored-by: ElePT <57907331+ElePT@users.noreply.github.com> * add variance to callback * use std_dev in callback rather than variance * formatting * return variance and shots in callback * return full metadata in callback * Move validation functions to algorithms/utils * correct the callback attribute documentation * correct the callback attribute typehint docstring * update VQE docstring * release note and pending-depreciate old algs * update vqe class docstring * Apply suggestions from code review Co-authored-by: ElePT <57907331+ElePT@users.noreply.github.com> * Do not copy ansatz * Note pending depreciation of old algs * fix docstrings and imports * Fix issues with building docs * Include OptimizerResult in VQEResult * Remove trailing whitespace * Fix math notation in docstring * estimate_obervables to return metadata @ElePT +VQE * add example in release note * Update evaluate_observables docstring * Fix observables_evaluator tests. Co-authored-by: ElePT <57907331+ElePT@users.noreply.github.com> * fix trotter_qrte tests and remove depreciation * formatting * remove unused import * Apply suggestions to docstring from code review Co-authored-by: Steve Wood <40241007+woodsp-ibm@users.noreply.github.com> * Update VQE docstring * Remove printing Co-authored-by: ElePT <57907331+ElePT@users.noreply.github.com> * Add parameters to estimate observables * Fix estimate obs. unit test * Update arg description * Update arg description * keep equation part of sentence * dict -> dict[str, Any] * Update qiskit/algorithms/optimizers/spsa.py Apply suggestion Imamichi-san * introduce FilterType and aux_operator_eigenvalues -> aux_operators_evaluated * Correct typehint Co-authored-by: Ikko Hamamura <ikkoham@users.noreply.github.com> * update vqe docstring and use old typing for type alias Co-authored-by: ElePT <57907331+ElePT@users.noreply.github.com> Co-authored-by: dlasecki <dal@zurich.ibm.com> Co-authored-by: Max Rossmannek <max.rossmannek@uzh.ch> Co-authored-by: Steve Wood <40241007+woodsp-ibm@users.noreply.github.com> Co-authored-by: ElePT <epenatap@gmail.com> Co-authored-by: Ikko Hamamura <ikkoham@users.noreply.github.com>
Summary
Adds a standard
VQE
implementation using anEstimator
primitive. Closes #8471.A
SamplingVQE
implementation that takes aSampler
primitive is introduced in PR #8669.Details and comments
This fresh implementation lives in
qiskit/algorithms/minimum_eigensolvers
. The prior implementation may still be found inqiskit/algorithms/minimum_eigen_solvers
.The following elements have been removed:
Sampler
and the optimal point from VQE outside the algorithm.The callback from the energy evaluation. Instead the callback should be attached to the optimizer which can allow to additionally evaluate the energy at the current point.After discussing with @woodsp-ibm, I decided to add back the callback on VQE as this may be simpler for users. Callbacks are not universally supplied by all optimizers.setting
property andprint_settings
method. No other algorithm has these.initial_point
, which requires it for theVariationalAlgorithm
interface.expectation
andinclude_custom
. These are not required anymore when using anEstimator
.