Remove a validation check of circuits for `BaseSampler` (follow-up of #8678) by t-imamichi · Pull Request #8708 · Qiskit/qiskit

t-imamichi · 2022-09-08T14:40:33Z

Summary

#8678 introduced some validation check of circuits for BaseSampler.
But, @nonhermitian suggested relaxing a check to support mid-circuit measurements in the future.
See the thread for details. #8678 (comment)

Follow-up #8678

Details and comments

The check of measurements is moved from BaseSampler to Sampler (the reference impl) because Sampler requires it. This is the revert of a change of #8678.

qiskit-bot · 2022-09-08T14:40:38Z

Thank you for opening a new pull request.

Before your PR can be merged it will first need to pass continuous integration tests and be reviewed. Sometimes the review process can be slow, so please be patient.

While you're waiting, please feel free to review other open PRs. While only a subset of people are authorized to approve pull requests for merging, everyone is encouraged to review open pull requests. Doing reviews helps reduce the burden on the core team and helps make the project's code better for everyone.

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

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

coveralls · 2022-09-08T15:40:48Z

Pull Request Test Coverage Report for Build 3135998431

3 of 3 (100.0%) changed or added relevant lines in 2 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage increased (+0.04%) to 84.406%

Totals
Change from base Build 3132502212:	0.04%
Covered Lines:	59614
Relevant Lines:	70628

💛 - Coveralls

ikkoham

LGTM. I want to wait @nonhermitian's review.

nonhermitian

I still think this has issues (see my one comment). I think the only check that one can make at this stage is to raise only if there is no classical bits at all. That way at least you know that there is nothing to measure into, guaranteeing a empty dict output

nonhermitian · 2022-09-13T09:27:25Z

    def _preprocess_circuit(circuit: QuantumCircuit):
        circuit = init_circuit(circuit)
        q_c_mapping = final_measurement_mapping(circuit)
+        if set(range(circuit.num_clbits)) != set(q_c_mapping.values()):


This looks like it is still going to raise if the number of clbits is not equal to the number of final measurements. That is to say that if I wrote to all my bits via mid-circuit measurements then this would be triggered.

This is part of the reference implementation based on quantum_info.Statevector. It requires final measurements. Statevector does not support mid-circuit measurements.

I updated the title of this PR to mention BaseSampler

But then you are blocking real-world use cases for a simulation method that does not scale. Does not seem like a fruitful direction to me.

The reference implementation was designed to be as simple as possible. Runtime primitives and Aer primitives are more appropriate for scaling, e.g., Qiskit/qiskit-aer#1531
This is similar to the relationship of BasicAer and Aer simulator. The reference implementation of primitives corresponds to BasicAer and Aer primitives corresponds to Aer simulator.

That is another way to think about it; Just overload all in Runtime. In some sense that is what would probably need to be done because a state vector simulator is quite far removed from a real hardware sampling routine

There is a PR #8668 working on backend-based Sampler. It is closer to real hardware sampling. Once it's merged, we can discuss whether we need the reference implementation.

This check is only part of the reference implementation of Sampler and not part of BaseSampler. So, other Sampler implementations does use this check. Other implementations are supposed to inherit only BaseSampler.

The reference implementation Sampler needs this check. So, I have to put it in BaseSampler (abstract class) or Sampler (reference implementation). I moved it from Sampler to BaseSampler in #8668. I want to revert it in this PR. If this PR is not merged, the check stays in BaseSampler and raises an error for all implementations not only the reference impl but also qiskit-ibm-runtime.

nonhermitian · 2022-09-13T10:05:41Z

For the record, I do not think there is a completely automated way to determine what measurements an end-user considers as part of the final output distributions. Dynamic circuits make this impossible in general.

ikkoham · 2022-09-13T10:23:12Z

Does Sampler need to support mid-circuit measurements in the first place? I thought mid-circuit measurement could not be supported the various error-mitigation methods. But I am not aware of any discussions around this, and I don't know the range of circuits Sampler can take (i.e. should Sampler support dynamic circuits?).

nonhermitian · 2022-09-13T10:27:52Z

I would say absolutely, otherwise how is it a "primitive"? One can correct some mid-circuit measurements provided the results of those measurements are aggregated. Several of us would like this, and I am re-tooling M3 to allow for it. However, the single-shot results cannot be; it would have to be corrected in HW.

t-imamichi · 2022-09-14T03:28:24Z

I still think this has issues (see my one comment). I think the only check that one can make at this stage is to raise only if there is no classical bits at all. That way at least you know that there is nothing to measure into, guaranteeing a empty dict output

This PR moves the check of classical bits and final measurement mapping from BaseSampler and Sampler (revert of #8668). So, this PR does the check only whether there is no classical bits at all.

t-imamichi · 2022-09-16T10:29:41Z

I resolved conflicts to merge main.

# Conflicts: # qiskit/primitives/base_sampler.py

ikkoham

I think it's good. Can I merge this? @nonhermitian

# Conflicts: # qiskit/primitives/base_sampler.py

t-imamichi · 2022-09-27T10:31:18Z

I fixed the conflict.

t-imamichi · 2022-09-27T13:31:09Z

This PR conflicts with #8760. So, I fixed a test with a note.

ikkoham

At least, this PR is better than before since the validation in BaseSampler is removed.
If it's not sufficient, let's fix it after RC release.

t-imamichi requested review from a team and ikkoham as code owners September 8, 2022 14:40

t-imamichi requested a review from nonhermitian September 8, 2022 14:40

t-imamichi force-pushed the followup-8678 branch from 1c2ea6d to 7403204 Compare September 8, 2022 14:55

t-imamichi mentioned this pull request Sep 8, 2022

Add input data check for Sampler Qiskit/qiskit-ibm-runtime#519

Merged

relax a check of measurements for Sampler

195aefd

t-imamichi force-pushed the followup-8678 branch from 7403204 to 195aefd Compare September 8, 2022 15:10

t-imamichi added Changelog: None Do not include in the GitHub Release changelog. mod: primitives Related to the Primitives module and removed Changelog: None Do not include in the GitHub Release changelog. labels Sep 13, 2022

t-imamichi added this to the 0.22 milestone Sep 13, 2022

ikkoham previously approved these changes Sep 13, 2022

View reviewed changes

nonhermitian reviewed Sep 13, 2022

View reviewed changes

t-imamichi changed the title ~~Remove a validation check of circuits for Sampler (follow-up of #8678)~~ Remove a validation check of circuits for BaseSampler (follow-up of #8678) Sep 13, 2022

t-imamichi dismissed ikkoham’s stale review via bdea96e September 16, 2022 10:29

Merge branch 'main' into followup-8678

3fc20ae

# Conflicts: # qiskit/primitives/base_sampler.py

t-imamichi force-pushed the followup-8678 branch from bdea96e to 3fc20ae Compare September 16, 2022 13:12

Merge branch 'main' into followup-8678

1898e79

ikkoham previously approved these changes Sep 26, 2022

View reviewed changes

Merge branch 'main' into followup-8678

ebcf87b

# Conflicts: # qiskit/primitives/base_sampler.py

fix a test

71ee01a

t-imamichi dismissed ikkoham’s stale review via 71ee01a September 27, 2022 13:28

mtreinish assigned ikkoham Sep 27, 2022

ikkoham approved these changes Sep 28, 2022

View reviewed changes

ikkoham added the automerge label Sep 29, 2022

Merge branch 'main' into followup-8678

be3f4e3

mergify Bot merged commit 5b35a3d into Qiskit:main Sep 29, 2022

t-imamichi deleted the followup-8678 branch October 3, 2022 09:38

garrison mentioned this pull request Feb 24, 2023

Sampler primitive in Terra does not support mid-circuit measurements #9657

Closed

t-imamichi mentioned this pull request Aug 21, 2023

Add validation to Sampler if there are no measurements #10642

Merged

Conversation

t-imamichi commented Sep 8, 2022

Summary

Details and comments

Uh oh!

qiskit-bot commented Sep 8, 2022

Uh oh!

coveralls commented Sep 8, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Test Coverage Report for Build 3135998431

💛 - Coveralls

Uh oh!

ikkoham left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nonhermitian left a comment

Choose a reason for hiding this comment

Uh oh!

nonhermitian Sep 13, 2022

Choose a reason for hiding this comment

Uh oh!

t-imamichi Sep 13, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

t-imamichi Sep 13, 2022

Choose a reason for hiding this comment

Uh oh!

nonhermitian Sep 13, 2022

Choose a reason for hiding this comment

Uh oh!

t-imamichi Sep 13, 2022

Choose a reason for hiding this comment

Uh oh!

nonhermitian Sep 13, 2022

Choose a reason for hiding this comment

Uh oh!

t-imamichi Sep 13, 2022

Choose a reason for hiding this comment

Uh oh!

t-imamichi Sep 13, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

t-imamichi Sep 14, 2022

Choose a reason for hiding this comment

Uh oh!

nonhermitian commented Sep 13, 2022

Uh oh!

ikkoham commented Sep 13, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nonhermitian commented Sep 13, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

t-imamichi commented Sep 14, 2022

Uh oh!

t-imamichi commented Sep 16, 2022

Uh oh!

ikkoham left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

t-imamichi commented Sep 27, 2022

Uh oh!

t-imamichi commented Sep 27, 2022

Uh oh!

ikkoham left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

coveralls commented Sep 8, 2022 •

edited

Loading

ikkoham left a comment •

edited

Loading

t-imamichi Sep 13, 2022 •

edited

Loading

t-imamichi Sep 13, 2022 •

edited

Loading

ikkoham commented Sep 13, 2022 •

edited

Loading

nonhermitian commented Sep 13, 2022 •

edited

Loading

ikkoham left a comment •

edited

Loading