qiskit icon indicating copy to clipboard operation
qiskit copied to clipboard

VQE implementation with estimator primitive

Open declanmillar opened this issue 2 years ago • 8 comments

Summary

Adds a standard VQE implementation using an Estimator primitive. Closes #8471.

A SamplingVQE implementation that takes a Sampler 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 in qiskit/algorithms/minimum_eigen_solvers.

The following elements have been removed:

  • The final "eigenstate". To reproduce this one may use a 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.
  • The setting property and print_settings method. No other algorithm has these.
  • All trivial getters and setters, just use public attributes instead (expect for initial_point, which requires it for the VariationalAlgorithm interface.
  • expectation and include_custom. These are not required anymore when using an Estimator.

declanmillar avatar Sep 07 '22 16:09 declanmillar

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:

  • @Qiskit/terra-core
  • @manoelmarques
  • @woodsp-ibm

qiskit-bot avatar Sep 07 '22 16:09 qiskit-bot

CLA assistant check
All committers have signed the CLA.

CLAassistant avatar Sep 07 '22 16:09 CLAassistant

Pull Request Test Coverage Report for Build 3135497477

  • 257 of 266 (96.62%) changed or added relevant lines in 11 files are covered.
  • 9 unchanged lines in 2 files lost coverage.
  • Overall coverage increased (+0.08%) to 84.451%

Changes Missing Coverage Covered Lines Changed/Added Lines %
qiskit/algorithms/minimum_eigensolvers/minimum_eigensolver.py 31 32 96.88%
qiskit/algorithms/minimum_eigensolvers/numpy_minimum_eigensolver.py 42 45 93.33%
qiskit/algorithms/minimum_eigensolvers/vqe.py 110 115 95.65%
<!-- Total: 257 266
Files with Coverage Reduction New Missed Lines %
qiskit/extensions/quantum_initializer/squ.py 2 79.78%
qiskit/algorithms/optimizers/qnspsa.py 7 87.3%
<!-- Total: 9
Totals Coverage Status
Change from base Build 3132502212: 0.08%
Covered Lines: 59852
Relevant Lines: 70872

💛 - Coveralls

coveralls avatar Sep 12 '22 16:09 coveralls

I note that estimate_observables now returns the variances and shots if obtained from metadata (and sets these to 0.0 otherwise). Should I add this for the main eigenvalue too? It seems odd to have this data for the aux operators and not the main one. I guess there are three ways of doing this:

  1. Make eigenvalue: tuple[complex, tuple[complex, int]], which will require a lot of downstream changes.
  2. Add a metadata property to the result class with the metadata pulled straight from the job.
  3. Add shots and variance properties to the result class. Any thoughts on this @woodsp-ibm, @mrossinek, @Cryoris?

declanmillar avatar Sep 16 '22 16:09 declanmillar

I note that estimate_observables now returns the variances and shots if obtained from metadata (and sets these to 0.0 otherwise). Should I add this for the main eigenvalue too?

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 https://github.com/Qiskit/qiskit-terra/pull/8683#discussion_r967314224 while perhaps keeping the extracted variance as a first class field.

#8105 may change things up.

woodsp-ibm avatar Sep 18 '22 18:09 woodsp-ibm

I note that estimate_observables now returns the variances and shots if obtained from metadata (and sets these to 0.0 otherwise). Should I add this for the main eigenvalue too? It seems odd to have this data for the aux operators and not the main one. I guess there are three ways of doing this:

  1. Make eigenvalue: tuple[complex, tuple[complex, int]], which will require a lot of downstream changes.
  2. Add a metadata property to the result class with the metadata pulled straight from the job.
  3. Add shots and variance properties to the result class. Any thoughts on this @woodsp-ibm, @mrossinek, @Cryoris?

I think that adding the metadata property (option 2) sounds like the most consistent option. I am not sure about the usefulness of also including the variance as a first class field, I think it would complicate things. Or @woodsp-ibm, were you referring to the aux_ops_eigenvalues that already return the variance?? I think it would make sense for them to mirror the main eigenvalues, and also return the auxiliary variance/shots as a metadata entry.

About https://github.com/Qiskit/qiskit-terra/pull/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.

ElePT avatar Sep 19 '22 11:09 ElePT

were you referring to the aux_ops_eigenvalues that already return the variance??

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.

woodsp-ibm avatar Sep 19 '22 20:09 woodsp-ibm

In thinking about the comments in a different PR #8695 I am wondering whether it would be a good idea to add OptimizerResult to the VQEResult. Generally as we have gone up the stack we added the the result of the algorithm being used to the result at that level. So a nature ground state result or an optimization you can look in and find the underlying result this was based on and any other info on that result. I am thinking we could do the same here - it would give access to optimizer results, from whatever optimizer was run, and you can access it or not as you choose. Maybe the place to add this would be in VariationalResult since that contains other optimizer values. @ElePT since this would be for VQD as well.

woodsp-ibm avatar Sep 23 '22 19:09 woodsp-ibm

It looks good overall though I wrote some minor comments.

t-imamichi avatar Sep 27 '22 10:09 t-imamichi