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

Fix BitArray.from_counts/from_samples to not fail for input with only 0 outcome and num_bits=None #12800

Merged
merged 3 commits into from
Jul 26, 2024

Conversation

aeddins-ibm
Copy link
Contributor

Summary

Partial fix for #12765.

That issue points out two problems with BitArray.from_counts / BitArray.from_samples. The more severe problem is that these methods fail with an error if the input (e.g. a counts dictionary) contains only the 0 outcome, and num_bits is left at the default value of None.

The methods try to infer num_bits as the number of bits needed to represent the largest outcome (e.g. bitstring) contained in the input data. If the largest outcome is 0 when converted to an integer (e.g. the bitstring "0"), the number of bits inferred is 0, which can be surprising, and also leads to an error.

This small PR interprets 0 as represented by 1 bit instead of by 0 bits, the suggested correct behavior in #12765.

The less severe problem, not fixed here, is that currently any information about num_bits present in the input bitstring lengths gets ignored. That problem seemed less trivial to me. I put some thoughts here.

@aeddins-ibm aeddins-ibm requested review from a team as code owners July 22, 2024 16:10
@qiskit-bot qiskit-bot added the Community PR PRs from contributors that are not 'members' of the Qiskit repo label Jul 22, 2024
@qiskit-bot
Copy link
Collaborator

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 following people are relevant to this code:

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

@coveralls
Copy link

coveralls commented Jul 22, 2024

Pull Request Test Coverage Report for Build 10102416610

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

Details

  • 2 of 2 (100.0%) changed or added relevant lines in 1 file are covered.
  • 846 unchanged lines in 45 files lost coverage.
  • Overall coverage decreased (-0.2%) to 89.7%

Files with Coverage Reduction New Missed Lines %
qiskit/qobj/common.py 1 95.83%
qiskit/circuit/library/standard_gates/swap.py 1 98.18%
qiskit/primitives/backend_sampler.py 1 98.86%
qiskit/circuit/duration.py 1 70.27%
qiskit/primitives/base/base_estimator.py 1 97.3%
qiskit/transpiler/passes/layout/sabre_pre_layout.py 1 98.88%
qiskit/transpiler/passes/routing/commuting_2q_gate_routing/commuting_2q_gate_router.py 1 99.12%
qiskit/assembler/disassemble.py 2 98.8%
qiskit/transpiler/passes/scheduling/alap.py 2 96.72%
qiskit/transpiler/passes/scheduling/asap.py 2 96.92%
Totals Coverage Status
Change from base Build 10043438194: -0.2%
Covered Lines: 66662
Relevant Lines: 74317

💛 - Coveralls

@ElePT ElePT added this to the 1.2.0 milestone Jul 23, 2024
@ElePT ElePT added mod: primitives Related to the Primitives module Changelog: Bugfix Include in the "Fixed" section of the changelog labels Jul 25, 2024
@ElePT
Copy link
Contributor

ElePT commented Jul 25, 2024

Hi @aeddins-ibm, could you add a short release note documenting the fix?

Copy link
Contributor

@ElePT ElePT left a comment

Choose a reason for hiding this comment

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

This fix looks straightforward, I think we should include it in 1.2 and backport it to 1.1

@ElePT ElePT added this pull request to the merge queue Jul 26, 2024
@ElePT
Copy link
Contributor

ElePT commented Jul 26, 2024

https://github.com/Mergifyio backport stable/1.1 stable/1.2

Copy link
Contributor

mergify bot commented Jul 26, 2024

backport stable/0.1.2 stable/1.2

❌ No backport have been created

  • Backport to branch stable/0.1.2 failed

GitHub error: Branch not found

Copy link
Contributor

mergify bot commented Jul 26, 2024

Copy link
Member

@t-imamichi t-imamichi left a comment

Choose a reason for hiding this comment

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

LGTM

Merged via the queue into Qiskit:main with commit c40ce83 Jul 26, 2024
15 checks passed
mergify bot pushed a commit that referenced this pull request Jul 26, 2024
…only `0` outcome and `num_bits=None` (#12800)

* interpret `0` as represented by 1 bit

* Test conversion of counts w only `0` outcome

* add release note

(cherry picked from commit c40ce83)
mergify bot pushed a commit that referenced this pull request Jul 26, 2024
…only `0` outcome and `num_bits=None` (#12800)

* interpret `0` as represented by 1 bit

* Test conversion of counts w only `0` outcome

* add release note

(cherry picked from commit c40ce83)
github-merge-queue bot pushed a commit that referenced this pull request Jul 26, 2024
…only `0` outcome and `num_bits=None` (#12800) (#12821)

* interpret `0` as represented by 1 bit

* Test conversion of counts w only `0` outcome

* add release note

(cherry picked from commit c40ce83)

Co-authored-by: aeddins-ibm <60495383+aeddins-ibm@users.noreply.github.com>
github-merge-queue bot pushed a commit that referenced this pull request Jul 26, 2024
…only `0` outcome and `num_bits=None` (#12800) (#12820)

* interpret `0` as represented by 1 bit

* Test conversion of counts w only `0` outcome

* add release note

(cherry picked from commit c40ce83)

Co-authored-by: aeddins-ibm <60495383+aeddins-ibm@users.noreply.github.com>
Procatv pushed a commit to Procatv/qiskit-terra-catherines that referenced this pull request Aug 1, 2024
…only `0` outcome and `num_bits=None` (Qiskit#12800)

* interpret `0` as represented by 1 bit

* Test conversion of counts w only `0` outcome

* add release note
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 Community PR PRs from contributors that are not 'members' of the Qiskit repo mod: primitives Related to the Primitives module
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

5 participants