Add Statevector-based SamplerV2 #11566

t-imamichi · 2024-01-15T14:09:51Z

Summary

Add Statevector-based Sampler V2 based on BaseSamplerV2 #11529.

This uses ongoing PRs #11543 #11552 and #11529 and we need to wait for them to be merged.
This PR will be rebased once they are merged.

previous version #11264

Details and comments

coveralls · 2024-01-15T14:40:17Z

Pull Request Test Coverage Report for Build 7571175775

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.02%) to 89.491%

Totals
Change from base Build 7561569421:	0.02%
Covered Lines:	59463
Relevant Lines:	66446

💛 - Coveralls

qiskit/primitives/statevector_sampler.py

t-imamichi · 2024-01-17T10:12:39Z

The file names and class names are tentative.

t-imamichi · 2024-01-17T10:16:50Z

I'm wondering shots=0 is allowed or not. Can BitArray handle shots=0?
If shots=0 is not allowed, we need to change the validation of SamplerPub.

qiskit-bot · 2024-01-17T10:18:46Z

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

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

qiskit/primitives/statevector_sampler.py

Co-authored-by: Ian Hincks <ian.hincks@gmail.com>

chriseclectic

Looks good, couple questions and suggestions. The only major change I would say is to change the seed behavior to give deterministic output to every run call with the same input pubs.

chriseclectic · 2024-01-17T15:01:51Z

qiskit/primitives/statevector_sampler.py

+    qreg_indices: list[int]
+
+
+class Sampler(BaseSamplerV2):


Do we want to call it Sampler or StatevectorSampler ? In 11227 the reference estimator is called StatevectorEstimator

I slightly prefer StatevectorSampler, but more importantly, they should have the same convention.

The reference impl of SamplerV1 is called just Sampler link. I'm wondering calling the new one SamplerV2 or StatevectorSampler. I will follow the convention of #11227.

qiskit/primitives/statevector_sampler.py

chriseclectic · 2024-01-17T15:04:50Z

qiskit/primitives/statevector_sampler.py

+    Simple implementation of :class:`BaseSamplerV2` with Statevector.
+    """
+
+    _DEFAULT_SHOTS: int = 512


Suggested change

_DEFAULT_SHOTS: int = 512

I would replace this with init kwarg

I suggested not putting it as a kwarg to discourage users from assuming that constructors are the "right" place to set shots. If it is placed in the constructor, consider calling it "default_shots". IMO, its fine right here as a class atribute, but if you both prefer it in the constructor, please go ahead.

Here is the original Ian's suggestion #11566 (comment), JFYI.
I made default_shots kwarg.

chriseclectic · 2024-01-17T15:09:39Z

qiskit/primitives/statevector_sampler.py

+        if isinstance(self._seed, np.random.Generator):
+            self._rng = self._seed
+        else:
+            self._rng = np.random.default_rng(self._seed)


You can seed a default_rng with another generator so this can always be np.random.default_rng(seed) even when seed is a generator. Eg

rng1 = np.random.default_rng(10) rng2 = np.random.default_rng(rng1) rng2.integers(10, size=10)

and this is the same as using a fixed seed for rng2. Hence you can just have

Suggested change

if isinstance(self._seed, np.random.Generator):

self._rng = self._seed

else:

self._rng = np.random.default_rng(self._seed)

self._rng = np.random.default_rng(self._seed)

To be deterministic, we don't need _rng and I put _seed directly to Statevector.

qiskit/primitives/statevector_sampler.py

chriseclectic · 2024-01-17T15:18:27Z

qiskit/primitives/statevector_sampler.py

+                final_state = Statevector(bound_circuit_to_instruction(bound_circuit))
+                final_state.seed(self._rng)
+                if qargs:
+                    samples = final_state.sample_memory(shots=pub.shots, qargs=qargs)


TODO For the future: we should add a sample method to Statevector (or make a helper function here) that directly samples in the BitArray format rather than constructing bit strings and converting them which is going to be more inefficient.

chriseclectic · 2024-01-17T15:28:55Z

test/python/primitives/test_statevector_sampler.py

+            self.assertEqual(len(result), 1)
+            self._assert_allclose(result[0].data.meas, np.array({1: self._shots}))
+
+    def test_circuit_with_multiple_cregs(self):


It might be good to also add a test where there are aliased registers, which can happen if you compose a circuit with conditionals into another circuit with a flat register. Eg for a bell teleportation like one

q = QuantumRegister(3, 'q') c1 = ClassicalRegister(1, 'c1') c2 = ClassicalRegister(1, 'c2') qc = QuantumCircuit(q, c1, c2) qc.h(2) qc.cx(2, 1) qc.cx(0, 1) qc.h(0) qc.measure(0, c1) qc.measure(1, c2) qc.z(2).c_if(c1, 1) qc.x(2).c_if(c2, 1) qc.draw() qc2 = QuantumCircuit(5, 5) qc2.compose(qc, [0, 2, 3], [2, 4], inplace=True) qc2.cregs

qc2 will have cregs

[ClassicalRegister(5, 'c'), ClassicalRegister(1, 'c4'), ClassicalRegister(1, 'c5')]

I found that register alias renamed register names ('c1' -> 'c4', 'c2' -> 'c5' in your example).
Is it possible to keep the register names?

Here is a obvious case.

from qiskit import QuantumCircuit, QuantumRegister, ClassicalRegister q = QuantumRegister(3, 'q') c1 = ClassicalRegister(1, 'c123456') c2 = ClassicalRegister(1, 'ccccccc') qc = QuantumCircuit(q, c1, c2) qc.h(2) qc.cx(2, 1) qc.cx(0, 1) qc.h(0) qc.measure(0, c1) qc.measure(1, c2) qc.z(2).c_if(c1, 1) qc.x(2).c_if(c2, 1) print(qc.cregs) # [ClassicalRegister(1, 'c123456'), ClassicalRegister(1, 'ccccccc')] qc2 = QuantumCircuit(5, 5) qc2.compose(qc, [0, 2, 3], [2, 4], inplace=True) print(qc2.cregs) # [ClassicalRegister(5, 'c'), ClassicalRegister(1, 'c0'), ClassicalRegister(1, 'c1')]

Anyways, I added aliased registers to databin though they are renamed.

qiskit/primitives/base/base_sampler.py

ihincks · 2024-01-17T20:39:05Z

qiskit/primitives/statevector_sampler.py

+    qreg_indices: list[int]
+
+
+class Sampler(BaseSamplerV2):


I slightly prefer StatevectorSampler, but more importantly, they should have the same convention.

ihincks · 2024-01-17T20:40:52Z

qiskit/primitives/statevector_sampler.py

+    Simple implementation of :class:`BaseSamplerV2` with Statevector.
+    """
+
+    _DEFAULT_SHOTS: int = 512


I suggested not putting it as a kwarg to discourage users from assuming that constructors are the "right" place to set shots. If it is placed in the constructor, consider calling it "default_shots". IMO, its fine right here as a class atribute, but if you both prefer it in the constructor, please go ahead.

qiskit/primitives/statevector_sampler.py

Co-authored-by: Ian Hincks <ian.hincks@gmail.com> Co-authored-by: Christopher J. Wood <cjwood@us.ibm.com>

t-imamichi · 2024-01-18T09:47:33Z

I think BitArray cannot support shots=0. How about updating SumplerPub.{validate, coerce} to check shots=0?
The change will be like t-imamichi/qiskit-terra@sampler-v2-rev2...t-imamichi:qiskit-terra:sampler-v2-shots

#11566 (comment)

t-imamichi · 2024-01-18T09:50:24Z

qiskit/primitives/statevector_sampler.py

-            creg = loc.registers[0]
            qbit = circuit.find_bit(item.qubits[0]).index
            if cbit in active_cbits and qbit in active_qubits:
-                mapping[creg] = qbit
+                for creg in loc.registers:
+                    mapping[creg] = qbit


Note: This change supports aliased classical registers.

t-imamichi · 2024-01-18T09:51:06Z

test/python/primitives/test_statevector_sampler.py

+        meas1 = result1[0].data.meas
+        result2 = sampler.run([bell], shots=self._shots).result()
+        meas2 = result2[0].data.meas
+        self._assert_allclose(meas1, meas2, rtol=0)


Note: Let rtol=0 to check two results are exactly same.

qiskit/primitives/statevector_sampler.py

ihincks

LGTM

Edit: I saw your comment about shots=0 after reading through the files and approving. I agree we should eliminate the possibility that shots=0 in the SamplerPub.

t-imamichi · 2024-01-18T14:08:18Z

Thanks. I updated this PR to raise ValueError with shots=0.

ihincks

Thanks!

chriseclectic

LGTM

1ucian0 · 2024-01-19T13:06:18Z

This PR tracks RFC 0016 https://github.com/Qiskit/RFCs/blob/master/0016-sampler-interface.md . With it's merge, can RFC 0016 be consider implemented?

ihincks · 2024-01-19T14:16:50Z

Yes, in fact I think #11529 could have been technically sufficient, but it's nice to have a reference implementation here, too.

* Add Statevector-based BaseSamplerV2 implementation Co-authored-by: Ian Hincks <ian.hincks@gmail.com> * Apply suggestions from code reivew Co-authored-by: Ian Hincks <ian.hincks@gmail.com> Co-authored-by: Christopher J. Wood <cjwood@us.ibm.com> * Apply suggestions from code review * shots should be positive * add tests of zero shots --------- Co-authored-by: Ian Hincks <ian.hincks@gmail.com> Co-authored-by: Christopher J. Wood <cjwood@us.ibm.com>

t-imamichi added on hold Can not fix yet mod: primitives Related to the Primitives module labels Jan 15, 2024

t-imamichi mentioned this pull request Jan 15, 2024

(WIP) Add Sampler V2 #11264

Closed

4 tasks

t-imamichi force-pushed the sampler-v2-rev2 branch from cf81d5e to 378bbf1 Compare January 15, 2024 14:18

ihincks reviewed Jan 15, 2024

View reviewed changes

qiskit/primitives/statevector_sampler.py Outdated Show resolved Hide resolved

ihincks reviewed Jan 15, 2024

View reviewed changes

qiskit/primitives/statevector_sampler.py Outdated Show resolved Hide resolved

t-imamichi commented Jan 16, 2024

View reviewed changes

qiskit/primitives/statevector_sampler.py Outdated Show resolved Hide resolved

ihincks reviewed Jan 16, 2024

View reviewed changes

qiskit/primitives/statevector_sampler.py Outdated Show resolved Hide resolved

kt474 mentioned this pull request Jan 16, 2024

Primitive interface updates for SamplerV2 Qiskit/qiskit-ibm-runtime#1313

Closed

t-imamichi force-pushed the sampler-v2-rev2 branch from b30fd58 to 8a2425d Compare January 17, 2024 10:10

t-imamichi force-pushed the sampler-v2-rev2 branch from 8a2425d to b23b4e0 Compare January 17, 2024 10:18

t-imamichi changed the title ~~[WIP] Add Statevector-based SamplerV2~~ Add Statevector-based SamplerV2 Jan 17, 2024

t-imamichi marked this pull request as ready for review January 17, 2024 10:18

t-imamichi requested review from a team as code owners January 17, 2024 10:18

t-imamichi commented Jan 17, 2024

View reviewed changes

qiskit/primitives/statevector_sampler.py Outdated Show resolved Hide resolved

Add Statevector-based BaseSamplerV2 implementation

e5f8cdb

Co-authored-by: Ian Hincks <ian.hincks@gmail.com>

t-imamichi force-pushed the sampler-v2-rev2 branch from b23b4e0 to e5f8cdb Compare January 17, 2024 10:23

chriseclectic requested changes Jan 17, 2024

View reviewed changes

ihincks reviewed Jan 17, 2024

View reviewed changes

ihincks mentioned this pull request Jan 17, 2024

Add reference implementation of EstimatorV2 #11227

Merged

ihincks removed the on hold Can not fix yet label Jan 17, 2024

ihincks reviewed Jan 17, 2024

View reviewed changes

qiskit/primitives/statevector_sampler.py Outdated Show resolved Hide resolved

Merge branch 'main' into sampler-v2-rev2

28e3498

t-imamichi force-pushed the sampler-v2-rev2 branch from 081a63c to 328cf06 Compare January 18, 2024 08:08

Apply suggestions from code reivew

077714c

Co-authored-by: Ian Hincks <ian.hincks@gmail.com> Co-authored-by: Christopher J. Wood <cjwood@us.ibm.com>

t-imamichi force-pushed the sampler-v2-rev2 branch from 328cf06 to 077714c Compare January 18, 2024 08:33

1ucian0 added this to the 1.0.0 milestone Jan 18, 2024

1ucian0 added the priority: high label Jan 18, 2024

t-imamichi requested review from ihincks and chriseclectic January 18, 2024 09:48

t-imamichi commented Jan 18, 2024

View reviewed changes

qiskit/primitives/statevector_sampler.py Show resolved Hide resolved

ihincks reviewed Jan 18, 2024

View reviewed changes

qiskit/primitives/statevector_sampler.py Outdated Show resolved Hide resolved

Apply suggestions from code review

a22c917

ihincks previously approved these changes Jan 18, 2024

View reviewed changes

t-imamichi added 2 commits January 18, 2024 23:04

shots should be positive

2b1f1dd

add tests of zero shots

b907ba7

t-imamichi dismissed ihincks’s stale review via b907ba7 January 18, 2024 14:07

ihincks approved these changes Jan 18, 2024

View reviewed changes

chriseclectic approved these changes Jan 18, 2024

View reviewed changes

chriseclectic added this pull request to the merge queue Jan 18, 2024

Merged via the queue into Qiskit:main with commit a0b2996 Jan 18, 2024
12 checks passed

t-imamichi deleted the sampler-v2-rev2 branch January 19, 2024 00:31

This was referenced Mar 6, 2024

RFC 0016: implemented Qiskit/RFCs#68

Open

Epic - Implementation of RFC 0016: SamplerV2 Qiskit/RFCs#71

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Statevector-based SamplerV2 #11566

Add Statevector-based SamplerV2 #11566

t-imamichi commented Jan 15, 2024 •

edited

coveralls commented Jan 15, 2024 •

edited

t-imamichi commented Jan 17, 2024

t-imamichi commented Jan 17, 2024

qiskit-bot commented Jan 17, 2024

chriseclectic left a comment

chriseclectic Jan 17, 2024

ihincks Jan 17, 2024

t-imamichi Jan 18, 2024

chriseclectic Jan 17, 2024

ihincks Jan 17, 2024

t-imamichi Jan 18, 2024 •

edited

chriseclectic Jan 17, 2024

t-imamichi Jan 18, 2024

chriseclectic Jan 17, 2024

chriseclectic Jan 17, 2024

t-imamichi Jan 18, 2024 •

edited

t-imamichi Jan 18, 2024

ihincks Jan 17, 2024

ihincks Jan 17, 2024

t-imamichi commented Jan 18, 2024

t-imamichi Jan 18, 2024 •

edited

t-imamichi Jan 18, 2024 •

edited

ihincks left a comment •

edited

t-imamichi commented Jan 18, 2024

ihincks left a comment

chriseclectic left a comment

1ucian0 commented Jan 19, 2024

ihincks commented Jan 19, 2024

Add Statevector-based SamplerV2 #11566

Add Statevector-based SamplerV2 #11566

Conversation

t-imamichi commented Jan 15, 2024 • edited

Summary

Details and comments

coveralls commented Jan 15, 2024 • edited

Pull Request Test Coverage Report for Build 7571175775

Warning: This coverage report may be inaccurate.

💛 - Coveralls

t-imamichi commented Jan 17, 2024

t-imamichi commented Jan 17, 2024

qiskit-bot commented Jan 17, 2024

chriseclectic left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

t-imamichi Jan 18, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

t-imamichi Jan 18, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

t-imamichi commented Jan 18, 2024

t-imamichi Jan 18, 2024 • edited

Choose a reason for hiding this comment

t-imamichi Jan 18, 2024 • edited

Choose a reason for hiding this comment

ihincks left a comment • edited

Choose a reason for hiding this comment

t-imamichi commented Jan 18, 2024

ihincks left a comment

Choose a reason for hiding this comment

chriseclectic left a comment

Choose a reason for hiding this comment

1ucian0 commented Jan 19, 2024

ihincks commented Jan 19, 2024

t-imamichi commented Jan 15, 2024 •

edited

coveralls commented Jan 15, 2024 •

edited

t-imamichi Jan 18, 2024 •

edited

t-imamichi Jan 18, 2024 •

edited

t-imamichi Jan 18, 2024 •

edited

t-imamichi Jan 18, 2024 •

edited

ihincks left a comment •

edited