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 validation to Sampler if there are no measurements #10642

Merged
merged 10 commits into from
Aug 29, 2023

Conversation

ikkoham
Copy link
Contributor

@ikkoham ikkoham commented Aug 16, 2023

Summary

This PR resolves TODO comment in BaseSampler.

Details and comments

@ikkoham ikkoham added stable backport potential The bug might be minimal and/or import enough to be port to stable Changelog: None Do not include in changelog mod: primitives Related to the Primitives module labels Aug 16, 2023
@ikkoham ikkoham requested review from t-imamichi and a team as code owners August 16, 2023 06:10
@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

Copy link
Contributor

@Cryoris Cryoris left a comment

Choose a reason for hiding this comment

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

That's a good check to have in place 👍🏻 Could you add a short releasenote for this, too?

qiskit/primitives/base/base_sampler.py Outdated Show resolved Hide resolved
qiskit/primitives/base/base_sampler.py Show resolved Hide resolved
@ikkoham
Copy link
Contributor Author

ikkoham commented Aug 16, 2023

Yes. I'll add a release note.

@coveralls
Copy link

coveralls commented Aug 16, 2023

Pull Request Test Coverage Report for Build 5921033642

  • 12 of 12 (100.0%) changed or added relevant lines in 1 file are covered.
  • 8 unchanged lines in 3 files lost coverage.
  • Overall coverage increased (+0.03%) to 87.274%

Files with Coverage Reduction New Missed Lines %
qiskit/primitives/sampler.py 1 95.65%
qiskit/pulse/library/waveform.py 3 93.75%
crates/qasm2/src/lex.rs 4 91.14%
Totals Coverage Status
Change from base Build 5902014500: 0.03%
Covered Lines: 74376
Relevant Lines: 85221

💛 - Coveralls

ikkoham and others added 3 commits August 16, 2023 15:57
@ikkoham ikkoham requested a review from Cryoris August 16, 2023 13:17
@ikkoham ikkoham added Changelog: Bugfix Include in the "Fixed" section of the changelog and removed Changelog: None Do not include in changelog labels Aug 18, 2023
@Cryoris
Copy link
Contributor

Cryoris commented Aug 18, 2023

LGTM, but I'm not sure it qualifies a bugfix and backport as this PR is technically an additional safety measure and not fixing a broken behavior. Is this something we need for 0.25.2 or is the next feature release enough?

@ikkoham
Copy link
Contributor Author

ikkoham commented Aug 21, 2023

@Cryoris Thank. Yes, you're right. This is not a feature bug fix, but I believe this is a error message bug or performance bug fix.
First, there were inappropriate error messages QiskitError: No counts for experiment \"0\" in qiskit-ibm-runtime. This PR improves a UX. Also, from another perspective, it should be meant to fix a performance bug, since errors before and after quantum computation execution consume very different amounts of resources.

@ikkoham
Copy link
Contributor Author

ikkoham commented Aug 21, 2023

This PR requires primitives team review. @t-imamichi or Mariana

@t-imamichi
Copy link
Member

I investigated the history of circuit validation. I relaxed one in BaseSampler to support dynamic circuits (see #8708). Does this PR resolve the issue, perhaps?

@t-imamichi
Copy link
Member

t-imamichi commented Aug 21, 2023

Details

I used to add a check of measurements #8678, but it was removed for mid-circuit measurement support in the future #8708. As a result, some Sampler implementations such as BackendSampler, Aer Sampler, and Runtime Sampler, raise an error when invoking job.result() as follows. This PR applies a check of measurement at run method to raise an error before submitting a job.

from qiskit import QuantumCircuit
from qiskit.primitives import Sampler, BackendSampler
from qiskit_aer import AerSimulator
from qiskit_aer.primitives import Sampler as AerSampler
from qiskit_ibm_runtime import Sampler as RuntimeSampler, QiskitRuntimeService, Session


qc = QuantumCircuit(1, 1)
qc.h(0)

if True:
    sampler = BackendSampler(backend=AerSimulator())
elif False:
    sampler = AerSampler()
else:
    service = QiskitRuntimeService(instance="ibm-q/open/main")
    backend = "ibmq_qasm_simulator"
    session = Session(service=service, backend=backend)
    sampler = RuntimeSampler(session=session)

job = sampler.run(qc, shots=100)
result = job.result()  # raise an error at result method
print(result)

output (main branch)

Traceback (most recent call last):
  File "/Users/ima/tasks/3_2023/qiskit/terra/tmp/sampler.py", line 22, in <module>
    result = job.result()
  File "/Users/ima/tasks/3_2023/qiskit/terra/qiskit/primitives/primitive_job.py", line 55, in result
    return self._future.result()
  File "/opt/homebrew/Cellar/python@3.10/3.10.12_1/Frameworks/Python.framework/Versions/3.10/lib/python3.10/concurrent/futures/_base.py", line 458, in result
    return self.__get_result()
  File "/opt/homebrew/Cellar/python@3.10/3.10.12_1/Frameworks/Python.framework/Versions/3.10/lib/python3.10/concurrent/futures/_base.py", line 403, in __get_result
    raise self._exception
  File "/opt/homebrew/Cellar/python@3.10/3.10.12_1/Frameworks/Python.framework/Versions/3.10/lib/python3.10/concurrent/futures/thread.py", line 58, in run
    result = self.fn(*self.args, **self.kwargs)
  File "/Users/ima/tasks/3_2023/qiskit/terra/qiskit/primitives/backend_sampler.py", line 149, in _call
    return self._postprocessing(result, bound_circuits)
  File "/Users/ima/tasks/3_2023/qiskit/terra/qiskit/primitives/backend_sampler.py", line 154, in _postprocessing
    counts = _prepare_counts(result)
  File "/Users/ima/tasks/3_2023/qiskit/terra/qiskit/primitives/backend_estimator.py", line 79, in _prepare_counts
    count = res.get_counts()
  File "/Users/ima/tasks/3_2023/qiskit/terra/qiskit/result/result.py", line 289, in get_counts
    raise QiskitError(f'No counts for experiment "{repr(key)}"')
qiskit.exceptions.QiskitError: 'No counts for experiment "0"'

output with this PR

Traceback (most recent call last):
  File "/Users/ima/tasks/3_2023/qiskit/terra/tmp/sampler.py", line 21, in <module>
    job = sampler.run(qc, shots=100)
  File "/Users/ima/tasks/3_2023/qiskit/terra/qiskit/primitives/base/base_sampler.py", line 134, in run
    circuits = self._validate_circuits(circuits)
  File "/Users/ima/tasks/3_2023/qiskit/terra/qiskit/primitives/base/base_sampler.py", line 176, in _validate_circuits
    raise ValueError(
ValueError: The 0-th circuit does not have Measure instruction. Without measurements, the circuit cannot be sampled from.

t-imamichi
t-imamichi previously approved these changes Aug 21, 2023
@t-imamichi
Copy link
Member

LGTM. But, let's wait for Mariana's review too.

@coveralls
Copy link

Pull Request Test Coverage Report for Build 5995648228

  • 12 of 12 (100.0%) changed or added relevant lines in 1 file are covered.
  • 35 unchanged lines in 3 files lost coverage.
  • Overall coverage decreased (-0.04%) to 87.245%

Files with Coverage Reduction New Missed Lines %
qiskit/primitives/sampler.py 1 95.65%
crates/qasm2/src/lex.rs 4 91.14%
crates/qasm2/src/parse.rs 30 95.74%
Totals Coverage Status
Change from base Build 5985235181: -0.04%
Covered Lines: 74265
Relevant Lines: 85122

💛 - Coveralls

@t-imamichi t-imamichi added this pull request to the merge queue Aug 29, 2023
Merged via the queue into Qiskit:main with commit 3bd1d46 Aug 29, 2023
14 checks passed
mergify bot pushed a commit that referenced this pull request Aug 29, 2023
* add validation

* Update qiskit/primitives/base/base_sampler.py

Co-authored-by: Julien Gacon <gaconju@gmail.com>

* add reno

* add test

* reversed iterate

---------

Co-authored-by: Julien Gacon <gaconju@gmail.com>
Co-authored-by: Takashi Imamichi <31178928+t-imamichi@users.noreply.github.com>
(cherry picked from commit 3bd1d46)
github-merge-queue bot pushed a commit that referenced this pull request Aug 29, 2023
* add validation

* Update qiskit/primitives/base/base_sampler.py

Co-authored-by: Julien Gacon <gaconju@gmail.com>

* add reno

* add test

* reversed iterate

---------

Co-authored-by: Julien Gacon <gaconju@gmail.com>
Co-authored-by: Takashi Imamichi <31178928+t-imamichi@users.noreply.github.com>
(cherry picked from commit 3bd1d46)

Co-authored-by: Ikko Hamamura <ikkoham@users.noreply.github.com>
@1ucian0
Copy link
Member

1ucian0 commented Aug 29, 2023

i just saw the email of this closing. Is this PR related to #10706 ?

@t-imamichi
Copy link
Member

No. This PR addresses a different issue.

SamD-1998 pushed a commit to SamD-1998/qiskit-terra that referenced this pull request Sep 7, 2023
* add validation

* Update qiskit/primitives/base/base_sampler.py

Co-authored-by: Julien Gacon <gaconju@gmail.com>

* add reno

* add test

* reversed iterate

---------

Co-authored-by: Julien Gacon <gaconju@gmail.com>
Co-authored-by: Takashi Imamichi <31178928+t-imamichi@users.noreply.github.com>
@ikkoham ikkoham deleted the primitives/sampler-with-no-measure branch September 21, 2023 02:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Changelog: Bugfix Include in the "Fixed" section of the changelog mod: primitives Related to the Primitives module stable backport potential The bug might be minimal and/or import enough to be port to stable
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants