-
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
Add BackendSamplerV2 #11928
Add BackendSamplerV2 #11928
Conversation
One or more of the the following people are requested to review this:
|
Pull Request Test Coverage Report for Build 8297352993Details
💛 - Coveralls |
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.
Basically LGTM, but we need more reviews.
|
||
def _validate_pubs(self, pubs: list[SamplerPub]): | ||
for i, pub in enumerate(pubs): | ||
if len(pub.circuit.cregs) == 0: |
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.
Do we need to check for measure instruction? Should we rather check measure instruction?
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 we need more validations. This is basically same as the check of StatevectorSampler
.
qiskit/qiskit/primitives/statevector_sampler.py
Lines 161 to 166 in ff2bbfe
if any(len(pub.circuit.cregs) == 0 for pub in coerced_pubs): | |
warnings.warn( | |
"One of your circuits has no output classical registers and so the result " | |
"will be empty. Did you mean to add measurement instructions?", | |
UserWarning, | |
) |
I tested this in the following way: with Aer: from qiskit import QuantumCircuit, transpile
from qiskit_aer import Aer
from qiskit.primitives import BackendSamplerV2
qasm_simulator = Aer.get_backend('qasm_simulator')
qasm_simulator_sampler = BackendSamplerV2(backend=qasm_simulator)
circuit = QuantumCircuit(2)
circuit.h(0)
circuit.cx(0, 1)
circuit.measure_all()
jobs = qasm_simulator_sampler.run([transpile(circuit, qasm_simulator)])
jobs.result()[0].data.meas.get_counts() with fake backend (V1 and V2): from qiskit import QuantumCircuit, transpile
from qiskit_ibm_runtime.fake_provider import FakeAlmadenV2
from qiskit.primitives import BackendSamplerV2
fake_almaden = FakeAlmadenV2()
fake_almaden_sampler = BackendSamplerV2(backend=fake_almaden)
circuit = QuantumCircuit(2)
circuit.h(0)
circuit.cx(0, 1)
circuit.measure_all()
jobs = fake_almaden_sampler.run([transpile(circuit, fake_almaden)])
print(jobs.result()[0].data.meas.get_counts()) It works as expected. I was surprised with the following errors:
|
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 find these tests hard to read. At the same time, they are based on test/python/primitives/test_backend_sampler.py
, which I also find hard to read. So probably for a follow up.
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.
Maybe an example with Fake7QPulseV1
similar to #11928 (comment) ?
Thank you for your review, Luciano. I add some comments to your surprises.
|
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.
Thank you @t-imamichi I only have a few minor comments. Other than those, this implimentation LGTM.
Co-authored-by: Ian Hincks <[email protected]> Co-authored-by: Ikko Hamamura <[email protected]>
I added |
I added |
features: | ||
- | | ||
The implementation :class:`~.BackendSamplerV2` of :class:`~.BaseSamplerV2` was added. | ||
This requires :class:`~.BackendV2` object that supports ``memory`` option to computes bitstrings. |
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.
Do you mind adding an example like #11928 (comment) ?
(pqc2, [0, 1, 2, 3, 4, 5, 6, 7], {0: 1898, 1: 6864, 2: 928, 3: 311}) | ||
) # case 6 | ||
|
||
def _assert_allclose(self, bitarray: BitArray, target: NDArray | BitArray, rtol=1e-1, atol=5e2): |
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.
with seeding, to you still need this _assert_allclose
?
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.
Yes. It is necessary because unit tests use both an exact simulator and noise simulators.
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.
Some non-blocking comments. It they can be addressed in this PR, great. Otherwise, in a follow up
I updated the code based on #11931 as follows.
|
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.
Let's merge it! Thanks!
Fixed a conflict |
Summary
This PR adds
BackendSamplerV2
as aBaseSamplerV2
implementation using a backend.This requires
BackendV2
object that supportsmemory
option.I made tests based on those for
StatevectorSampler
.Notes
I intentionally specified onlyBackendV2
asbackend
option though it still supportsBackendV1
because I think we need to support only the latest version of backends. If requested, I can addBackendV1
.BackendV1
as well asBackendV2
support.Details and comments