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 primitives.containers.BitArray container class #11542

Merged
merged 9 commits into from
Jan 12, 2024

Conversation

ihincks
Copy link
Contributor

@ihincks ihincks commented Jan 10, 2024

Summary

This PR introduces the BitArray class whose purpose is to store an array of bit values, and be the fundamental result type inside for SamplerV2 for those circuit outputs that are bit valued.

Instances of this object contain a single, contiguous block of data that represents an array of bitstrings. The last axis is over packed bits, the second last axis is over shots, and the preceding axes correspond to the shape of the pub that was executed to sample these bits.

There are helper functions for converting from/to other formats, as well as for performing bitwise operations directly on the data chunk.

Details and comments

See also Qiskit/RFCs#56

@ihincks ihincks requested review from a team as code owners January 10, 2024 20:07
@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

@ihincks ihincks added mod: primitives Related to the Primitives module priority: high labels Jan 10, 2024
@ihincks ihincks mentioned this pull request Jan 10, 2024
@coveralls
Copy link

coveralls commented Jan 10, 2024

Pull Request Test Coverage Report for Build 7505848919

Warning: This coverage report may be inaccurate.

We've detected an issue with your CI configuration that might affect the accuracy of this pull request's coverage report.
To ensure accuracy in future PRs, please see these guidelines.
A quick fix for this PR: rebase it; your next report should be accurate.

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+1.8%) to 89.354%

Totals Coverage Status
Change from base Build 7476756860: 1.8%
Covered Lines: 59542
Relevant Lines: 66636

💛 - Coveralls

@chriseclectic
Copy link
Member

I think the main open question is do we use shots or samples terminology here and in SamplerV2?

@t-imamichi
Copy link
Member

This PR looks good. I wait for the discussion of shots v.s. samples.

@ihincks
Copy link
Contributor Author

ihincks commented Jan 11, 2024

I am in favour of using shots.

Pros:

  • long standing tradition in quantum to mean individual outcomes measured from a system
  • long standing convention in IBM APIs
  • samples is used elsewhere in our stack to refer to randomly chosen circuits, etc

Cons:

  • The thing is called Sampler and so it's catchy for it to produce samples

IMO these pros outweigh the cons and is the path of least confusion.

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.

I have to side with @ihincks here, shots is the established convention, and I believe that changing the term will be more confusing than beneficial. On top of this, "samples" is already pretty overloaded within IBM quantum projects, for example, to refer to the sub-experiments using circuit knitting techniques, or to refer to pulse samples. A quick search of samples on the qiskit repo yields 69 results in the codebase.

@chriseclectic
Copy link
Member

I think "samples" being used heavily in the pulse layer is a good argument for sticking with "shots". I would also say we could go further and look to remove the terminology of using samples in twirling/circuit knitting and use a different name eg "randomizations" would be more descriptive for twirling

Copy link
Member

@chriseclectic chriseclectic left a comment

Choose a reason for hiding this comment

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

Couple of comments

qiskit/primitives/containers/bit_array.py Show resolved Hide resolved
qiskit/primitives/containers/bit_array.py Outdated Show resolved Hide resolved
qiskit/primitives/containers/bit_array.py Outdated Show resolved Hide resolved
qiskit/primitives/containers/bit_array.py Outdated Show resolved Hide resolved
qiskit/primitives/containers/bit_array.py Outdated Show resolved Hide resolved
qiskit/primitives/containers/bit_array.py Outdated Show resolved Hide resolved
qiskit/primitives/containers/bit_array.py Outdated Show resolved Hide resolved
qiskit/primitives/containers/bit_array.py Show resolved Hide resolved
Copy link
Member

@chriseclectic chriseclectic left a comment

Choose a reason for hiding this comment

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

LGTM

@chriseclectic chriseclectic added this pull request to the merge queue Jan 12, 2024
Merged via the queue into Qiskit:main with commit e573245 Jan 12, 2024
13 checks passed
@ihincks ihincks deleted the primitives/bit-array branch January 12, 2024 20:21
ShellyGarion pushed a commit to ShellyGarion/qiskit-terra that referenced this pull request Jan 18, 2024
* Add BitArray class and tests

This reverts commit bb5b9e1.

* change samples->shots for consistency with SamplerV2

* continue renaming samples>shots

* final rename of samples -> shots

* Update qiskit/primitives/containers/bit_array.py

Co-authored-by: Christopher J. Wood <[email protected]>

* Apply suggestions from code review

Co-authored-by: Christopher J. Wood <[email protected]>

* update the constructor to demand ndarray with unit8

* change behaviour of get_counts() as suggested by Chris

* fix linting

---------

Co-authored-by: Christopher J. Wood <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
mod: primitives Related to the Primitives module priority: high
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants