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

Add BaseEstimatorV2 #11527

Merged
merged 14 commits into from
Jan 17, 2024
Merged

Conversation

chriseclectic
Copy link
Member

@chriseclectic chriseclectic commented Jan 9, 2024

Summary

Depends on #11524

Adds BaseEstimatorV2 class

Details and comments

BaseEstimatorV2 is taken from #11227 with the following modifications:

  • Adds a precision attribute to BaseEstimatorV2 and EstimatorPub
  • Removes Options from BaseEstimatorV2
  • Removes unnecessary BasePrimitiveV2 and BasePub classes

The precision attribute is intended to be the API element for controlling estimator precision in algorithms or applications (eg VQE). Previously this intended to be done via shots in Options, however as pointed out in review Options do not define an actual interface API since subclasses can implement them however they like. Subclasses can still implement options (and the runtime estimator will), but they are no longer part of the base API, but rather specific to how an individual estimator can be configured. This means the only API elements of an initialized EstimatorV2 for use in applications are a run method, and a precision getter/setter.

@chriseclectic chriseclectic added priority: high mod: primitives Related to the Primitives module labels Jan 9, 2024
@qiskit-bot
Copy link
Collaborator

One or more of the the following people are requested to review this:

  • @Qiskit/terra-core
  • @ajavadia
  • @ikkoham
  • @levbishop
  • @t-imamichi

@coveralls
Copy link

coveralls commented Jan 9, 2024

Pull Request Test Coverage Report for Build 7560304464

Warning: This coverage report may be inaccurate.

We've detected an issue with your CI configuration that might affect the accuracy of this pull request's coverage report.
To ensure accuracy in future PRs, please see these guidelines.
A quick fix for this PR: rebase it; your next report should be accurate.

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.1%) to 89.455%

Totals Coverage Status
Change from base Build 7553630700: 0.1%
Covered Lines: 59341
Relevant Lines: 66336

💛 - Coveralls

@chriseclectic chriseclectic mentioned this pull request Jan 9, 2024
Copy link
Contributor

@ihincks ihincks left a comment

Choose a reason for hiding this comment

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

This review is a string of docstring touch-ups.

qiskit/primitives/base/base_estimator.py Outdated Show resolved Hide resolved
qiskit/primitives/base/base_estimator.py Outdated Show resolved Hide resolved
qiskit/primitives/base/base_estimator.py Outdated Show resolved Hide resolved
qiskit/primitives/containers/estimator_pub.py Outdated Show resolved Hide resolved
qiskit/primitives/containers/estimator_pub.py Outdated Show resolved Hide resolved
qiskit/primitives/containers/estimator_pub.py Outdated Show resolved Hide resolved
qiskit/primitives/containers/estimator_pub.py Outdated Show resolved Hide resolved
qiskit/primitives/base/base_estimator.py Outdated Show resolved Hide resolved
qiskit/primitives/base/base_estimator.py Outdated Show resolved Hide resolved
qiskit/primitives/containers/estimator_pub.py Outdated Show resolved Hide resolved
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.

Thank you @chriseclectic. Just one comment (question).

qiskit/primitives/base/base_estimator.py Outdated Show resolved Hide resolved
@1ucian0
Copy link
Member

1ucian0 commented Jan 10, 2024

One of the TODOs introduced in #11524 seems to be depending on a closed issue. Can that TODO be addressed in this PR?

        # TODO str will be used for internal data (_kwvals) instead of Parameter.
        # This requires https://github.com/Qiskit/qiskit/issues/7107

@chriseclectic chriseclectic added this to the 1.0.0 milestone Jan 10, 2024
Copy link
Contributor

@jyu00 jyu00 left a comment

Choose a reason for hiding this comment

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

What about estimator job results? The PubResult container doesn't really tell me what exactly data is returned.

qiskit/primitives/base/base_estimator.py Outdated Show resolved Hide resolved
qiskit/primitives/containers/estimator_pub.py Outdated Show resolved Hide resolved
@jyu00
Copy link
Contributor

jyu00 commented Jan 10, 2024

Removes Options from BaseEstimatorV2

Why was options removed from the base class? I understand the desire to keep the base class simple, but the user feedback we got was that it's preferred to have some consistency (e.g. same options related attributes/methods) across different primitive implementations. This was also documented in the RFC.

Copy link
Contributor

@ihincks ihincks left a comment

Choose a reason for hiding this comment

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

Looks like we'll need some tests for EstimatorPub too.

qiskit/primitives/base/base_estimator.py Show resolved Hide resolved
qiskit/primitives/base/base_estimator.py Show resolved Hide resolved
qiskit/primitives/base/base_estimator.py Show resolved Hide resolved
qiskit/primitives/base/base_estimator.py Outdated Show resolved Hide resolved
qiskit/primitives/base/base_estimator.py Show resolved Hide resolved
qiskit/primitives/containers/estimator_pub.py Show resolved Hide resolved
qiskit/primitives/containers/estimator_pub.py Show resolved Hide resolved
qiskit/primitives/containers/estimator_pub.py Show resolved Hide resolved
qiskit/primitives/containers/estimator_pub.py Show resolved Hide resolved
chriseclectic and others added 4 commits January 17, 2024 09:23
Taken from 11227, but modified to
- remove options from BaseEstimatorV2
- add precision attribute to BaseEstimatorV2 and EstimatorPub
- remove BasePrimitiveV2 and BasePub

Co-Authored-By: Ikko Hamamura <[email protected]>
Co-Authored-By: Ian Hincks <[email protected]>
Copy link
Contributor

@ihincks ihincks left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks all.

@ihincks ihincks added this pull request to the merge queue Jan 17, 2024
Merged via the queue into Qiskit:main with commit e16336b Jan 17, 2024
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
mod: primitives Related to the Primitives module priority: high
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants