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 BaseSamplerV2 #11529

Merged
merged 11 commits into from Jan 17, 2024
Merged

Add BaseSamplerV2 #11529

merged 11 commits into from Jan 17, 2024

Conversation

chriseclectic
Copy link
Member

@chriseclectic chriseclectic commented Jan 9, 2024

Summary

Adds BaseSaamplerV2 base class for V2 Sampler primitives

Depends on #11524

Details and comments

This splits out the base classes in from #11264 and makes the following changes

  • Adds shots attributes run kwarg to BaseSamplerV2
  • Adds shots to SamplerPub
  • Removes options from BaseSamplerV2 (subclasses are free to implement their own options if desired)
    See Add BaseEstimatorV2 #11527 for motivation for these changes.

@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 7547884544

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

Totals Coverage Status
Change from base Build 7547543556: 0.04%
Covered Lines: 59443
Relevant Lines: 66515

💛 - Coveralls

@@ -200,3 +258,38 @@ def parameters(self) -> tuple[ParameterView, ...]:
List of the parameters in each quantum circuit.
"""
return tuple(self._parameters)


BaseSampler = BaseSamplerV1
Copy link
Member Author

Choose a reason for hiding this comment

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

@jakelishman Is this the sort of thing we should have a from __future__ import <what?> so that BaseSampler gets set to BaseSamplerV2 instead? And that if you import BaseSampler without this import it raises a deprecation warning of some sort using the same trick you used in the other PRs and returns BaseSamplerV1 for it in those cases?

Copy link
Member Author

Choose a reason for hiding this comment

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

I guess for 1.0 we might want a future deprecation warning rather than actual deprecation warning for the V1 primitives, and then the actual warning on 1.1?

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

Moved shots to SamplerPub and the BaseSampler.run method to simplify the intended interface contract.

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.

Thanks, lookin' good.

qiskit/primitives/base/base_sampler.py Outdated Show resolved Hide resolved
qiskit/primitives/base/base_sampler.py Outdated Show resolved Hide resolved
qiskit/primitives/base/base_sampler.py Outdated Show resolved Hide resolved
qiskit/primitives/base/base_sampler.py Outdated Show resolved Hide resolved
qiskit/primitives/base/base_sampler.py Outdated Show resolved Hide resolved
qiskit/primitives/containers/bit_array.py Outdated Show resolved Hide resolved
qiskit/primitives/containers/sampler_pub.py Outdated Show resolved Hide resolved
qiskit/primitives/containers/sampler_pub.py Outdated Show resolved Hide resolved
qiskit/primitives/containers/sampler_pub.py Outdated Show resolved Hide resolved
qiskit/primitives/containers/sampler_pub.py Show resolved Hide resolved
Args:
pubs: An iterable of pub-like objects. For example, a list of circuits
or tuples ``(circuit, parameter_values)``.
shots: The total number of shots to sample for each :class:`.SamplerPub`.
Copy link
Member

Choose a reason for hiding this comment

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

If SamplerV2 has an execution option and the number of shots of the execution option and that of run method differ, I guess that of run will be used. Am I correct?

E.g.,

sampler = SamplerV2(options={"execution": {"shots": 10}})
result = sampler.run([circuit], shots=20).result()
# `result` contains 20 samples instead of 10 samples.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, in this situation the kwargs of run takes precedence over execution.shots. The latter should be thought of as the default value when run gets None.

raise ValueError("shots must be non-negative")

if isinstance(pub, SamplerPub):
if pub.shots is None and shots is not None:
Copy link
Member

Choose a reason for hiding this comment

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

I guess shots of this method is used as a default value not to overwrite the existing pub.shots.

Copy link
Contributor

Choose a reason for hiding this comment

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

That's right, pub.shots takes precedence.

qiskit/primitives/base/base_sampler.py Outdated Show resolved Hide resolved
qiskit/primitives/base/base_sampler.py Outdated Show resolved Hide resolved
qiskit/primitives/base/base_sampler.py Outdated Show resolved Hide resolved
qiskit/primitives/base/base_sampler.py Outdated Show resolved Hide resolved
qiskit/primitives/containers/sampler_pub.py Outdated Show resolved Hide resolved
qiskit/primitives/containers/sampler_pub.py Outdated Show resolved Hide resolved
qiskit/primitives/containers/sampler_pub.py Outdated Show resolved Hide resolved
qiskit/primitives/containers/sampler_pub.py Outdated Show resolved Hide resolved
chriseclectic and others added 2 commits January 16, 2024 16:36
Taken from 11264. Removes Options and adds `shots` attributes to BaseSamplerV2

Co-Authored-By: Takashi Imamichi <31178928+t-imamichi@users.noreply.github.com>
Co-Authored-By: Ian Hincks <2229105+ihincks@users.noreply.github.com>
Copy link
Member

@t-imamichi t-imamichi left a comment

Choose a reason for hiding this comment

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

LGTM

@t-imamichi t-imamichi added this pull request to the merge queue Jan 17, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jan 17, 2024
@t-imamichi t-imamichi added this pull request to the merge queue Jan 17, 2024
Merged via the queue into Qiskit:main with commit c799435 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.

None yet

6 participants