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.

Sign up for GitHub

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

Jump to bottom

Add BaseEstimatorV2 #11527

Merged
merged 14 commits into from Jan 17, 2024
Merged

Add BaseEstimatorV2 #11527

merged 14 commits into from Jan 17, 2024

Conversation

chriseclectic
Copy link
Member

@chriseclectic chriseclectic commented Jan 9, 2024 •
edited

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 •
edited

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
Merged
ihincks
ihincks reviewed Jan 9, 2024
View reviewed changes
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
@chriseclectic chriseclectic requested a review from a team as a code owner January 9, 2024 19:40
@ihincks ihincks mentioned this pull request Jan 9, 2024
Merged
ikkoham
ikkoham reviewed Jan 10, 2024
View reviewed changes
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
jyu00
jyu00 reviewed Jan 10, 2024
View reviewed changes
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.

This was referenced Jan 11, 2024
Closed
Closed
ihincks
ihincks reviewed Jan 16, 2024
View reviewed changes
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
@chriseclectic @ikkoham @ihincks
Add BaseEstimatorV2 class
fcf531c 
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 <ikkoham@users.noreply.github.com>
Co-Authored-By: Ian Hincks <2229105+ihincks@users.noreply.github.com>
@chriseclectic
Fix removal of BasePub
ab1c196
@chriseclectic
Remove precision from EstimatorPub
25018f8
@ihincks @chriseclectic
Add BaseEstimator._make_data_bin() static method
f178fb5
ihincks
ihincks approved these changes Jan 17, 2024
View reviewed changes
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 mentioned this pull request Jan 17, 2024
Closed
@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
Reviewers

@ikkoham ikkoham ikkoham left review comments

@jyu00 jyu00 jyu00 left review comments

@t-imamichi t-imamichi t-imamichi left review comments

@ihincks ihincks ihincks approved these changes

Assignees
No one assigned
Labels
mod: primitives Related to the Primitives module priority: high
Projects
None yet
Milestone
1.0.0
Development

Successfully merging this pull request may close these issues.

None yet
8 participants
@chriseclectic @qiskit-bot @coveralls @1ucian0 @jyu00 @ihincks @ikkoham @t-imamichi