qiskit icon indicating copy to clipboard operation
qiskit copied to clipboard

Remove a validation check of circuits for `BaseSampler` (follow-up of #8678)

Open t-imamichi opened this issue 2 years ago • 7 comments

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. https://github.com/Qiskit/qiskit-terra/pull/8678#discussion_r965797741

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.

t-imamichi avatar Sep 08 '22 14:09 t-imamichi

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

qiskit-bot avatar Sep 08 '22 14:09 qiskit-bot

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 Coverage Status
Change from base Build 3132502212: 0.04%
Covered Lines: 59614
Relevant Lines: 70628

💛 - Coveralls

coveralls avatar Sep 08 '22 15:09 coveralls

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.

nonhermitian avatar Sep 13 '22 10:09 nonhermitian

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?).

ikkoham avatar Sep 13 '22 10:09 ikkoham

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.

nonhermitian avatar Sep 13 '22 10:09 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

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 avatar Sep 14 '22 03:09 t-imamichi

I resolved conflicts to merge main.

t-imamichi avatar Sep 16 '22 10:09 t-imamichi

I fixed the conflict.

t-imamichi avatar Sep 27 '22 10:09 t-imamichi

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

t-imamichi avatar Sep 27 '22 13:09 t-imamichi