Skip to content

Add input data check for Sampler#519

Merged
kt474 merged 9 commits into
Qiskit:mainfrom
t-imamichi:sampler-error-check
Sep 21, 2022
Merged

Add input data check for Sampler#519
kt474 merged 9 commits into
Qiskit:mainfrom
t-imamichi:sampler-error-check

Conversation

@t-imamichi
Copy link
Copy Markdown
Member

@t-imamichi t-imamichi commented Sep 2, 2022

Summary

Add a validation check of circuits for Sampler. It raises an error if there is no classical bits in the input circuits.

I added the same test to Terra Qiskit/qiskit#8678 Qiskit/qiskit#8708
So, if we use the base class of primitives of Terra, we can remove these checks in qiskit-ibm-runtime.

Details and comments

@coveralls
Copy link
Copy Markdown

Pull Request Test Coverage Report for Build 2977882163

  • -7 of 11 (36.36%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-2.6%) to 61.736%

Changes Missing Coverage Covered Lines Changed/Added Lines %
qiskit_ibm_runtime/sampler.py 4 11 36.36%
Totals Coverage Status
Change from base Build 2972568577: -2.6%
Covered Lines: 3051
Relevant Lines: 4942

💛 - Coveralls

@coveralls
Copy link
Copy Markdown

coveralls commented Sep 2, 2022

Pull Request Test Coverage Report for Build 3091977741

  • 2 of 3 (66.67%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-2.5%) to 61.74%

Changes Missing Coverage Covered Lines Changed/Added Lines %
qiskit_ibm_runtime/qiskit/primitives/base_sampler.py 2 3 66.67%
Totals Coverage Status
Change from base Build 3085891949: -2.5%
Covered Lines: 3137
Relevant Lines: 5081

💛 - Coveralls

Copy link
Copy Markdown
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.

Two small comments, otherwise LGTM!

Comment thread qiskit_ibm_runtime/sampler.py Outdated
Comment thread qiskit_ibm_runtime/sampler.py Outdated
Copy link
Copy Markdown
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.

LGTM! For the Terra primitives we can essentially apply the same update, right?

@t-imamichi
Copy link
Copy Markdown
Member Author

Yeah, I will port this change to Terra next week.

@t-imamichi
Copy link
Copy Markdown
Member Author

I updated this PR and made the same checks to terra Qiskit/qiskit#8678.

@t-imamichi
Copy link
Copy Markdown
Member Author

I removed the validation check whether there is any classical bits not measured.
See Qiskit/qiskit#8708 for details.

@t-imamichi
Copy link
Copy Markdown
Member Author

Note that this check exists in Terra main branch's BaseSampler. So, if we plan to replace BaseSampler with Terra's, we don't need this PR.

Comment thread test/integration/test_sampler.py Outdated
@jyu00
Copy link
Copy Markdown
Collaborator

jyu00 commented Sep 19, 2022

Note that this check exists in Terra main branch's BaseSampler. So, if we plan to replace BaseSampler with Terra's, we don't need this PR.

We will use terra's BaseSampler eventually... but we need to terra to release first, or the two need to be released together (which is harder to coordinate...)

@t-imamichi
Copy link
Copy Markdown
Member Author

Yes, it's hard to sync with primitives in terra because it has been actively updated. Maybe copying a snapshot sometimes is a compromise.

@kt474 kt474 merged commit 408cbee into Qiskit:main Sep 21, 2022
@t-imamichi t-imamichi deleted the sampler-error-check branch September 21, 2022 14:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants