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

Bitarray postselect #12693

Merged
merged 51 commits into from
Jul 29, 2024
Merged

Bitarray postselect #12693

merged 51 commits into from
Jul 29, 2024

Conversation

aeddins-ibm
Copy link
Contributor

@aeddins-ibm aeddins-ibm commented Jun 28, 2024

Summary

Add method BitArray.postselect(), per issue #12688 .

[edit July 22, 2024: deleting outdated, distracting details from this description. See linked issue and the method docstring for more information.]

@aeddins-ibm aeddins-ibm requested review from a team as code owners June 28, 2024 23:11
@qiskit-bot qiskit-bot added the Community PR PRs from contributors that are not 'members' of the Qiskit repo label Jun 28, 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

@aeddins-ibm aeddins-ibm marked this pull request as draft June 28, 2024 23:12
@coveralls
Copy link

coveralls commented Jun 28, 2024

Pull Request Test Coverage Report for Build 9719523525

Details

  • 4 of 4 (100.0%) changed or added relevant lines in 1 file are covered.
  • 16 unchanged lines in 2 files lost coverage.
  • Overall coverage decreased (-0.008%) to 89.765%

Files with Coverage Reduction New Missed Lines %
crates/qasm2/src/lex.rs 4 92.88%
crates/qasm2/src/parse.rs 12 97.15%
Totals Coverage Status
Change from base Build 9718390567: -0.008%
Covered Lines: 64075
Relevant Lines: 71381

💛 - Coveralls

@coveralls
Copy link

coveralls commented Jun 29, 2024

Pull Request Test Coverage Report for Build 9725434498

Details

  • 4 of 4 (100.0%) changed or added relevant lines in 1 file are covered.
  • 4 unchanged lines in 2 files lost coverage.
  • Overall coverage increased (+0.009%) to 89.782%

Files with Coverage Reduction New Missed Lines %
qiskit/transpiler/passes/synthesis/unitary_synthesis.py 2 88.2%
crates/qasm2/src/lex.rs 2 93.38%
Totals Coverage Status
Change from base Build 9718390567: 0.009%
Covered Lines: 64087
Relevant Lines: 71381

💛 - Coveralls

@aeddins-ibm aeddins-ibm marked this pull request as ready for review June 29, 2024 17:44
@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

@ShellyGarion ShellyGarion removed the Community PR PRs from contributors that are not 'members' of the Qiskit repo label Jun 30, 2024
@t-imamichi t-imamichi added mod: primitives Related to the Primitives module Changelog: New Feature Include in the "Added" section of the changelog labels Jul 1, 2024
Copy link
Contributor

@ihincks ihincks left a comment

Choose a reason for hiding this comment

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

Thanks for opening the PR!

This change will also need a reno.

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

coveralls commented Jul 2, 2024

Pull Request Test Coverage Report for Build 9767937070

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

  • 4 of 4 (100.0%) changed or added relevant lines in 1 file are covered.
  • 630 unchanged lines in 20 files lost coverage.
  • Overall coverage increased (+0.03%) to 89.807%

Files with Coverage Reduction New Missed Lines %
qiskit/circuit/library/standard_gates/rxx.py 1 97.56%
qiskit/transpiler/passes/routing/commuting_2q_gate_routing/pauli_2q_evolution_commutation.py 1 97.92%
qiskit/circuit/library/standard_gates/rzx.py 1 97.56%
qiskit/circuit/library/standard_gates/ryy.py 2 95.12%
crates/accelerate/src/synthesis/permutation/utils.rs 2 97.94%
qiskit/circuit/library/standard_gates/rzz.py 2 94.74%
qiskit/circuit/library/standard_gates/p.py 3 96.55%
crates/qasm2/src/lex.rs 4 93.13%
crates/qasm2/src/parse.rs 6 97.61%
qiskit/visualization/circuit/circuit_visualization.py 16 73.43%
Totals Coverage Status
Change from base Build 9718390567: 0.03%
Covered Lines: 64545
Relevant Lines: 71871

💛 - Coveralls

@coveralls
Copy link

coveralls commented Jul 2, 2024

Pull Request Test Coverage Report for Build 9768517825

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

  • 4 of 4 (100.0%) changed or added relevant lines in 1 file are covered.
  • 631 unchanged lines in 20 files lost coverage.
  • Overall coverage increased (+0.03%) to 89.805%

Files with Coverage Reduction New Missed Lines %
qiskit/circuit/library/standard_gates/rxx.py 1 97.56%
qiskit/transpiler/passes/routing/commuting_2q_gate_routing/pauli_2q_evolution_commutation.py 1 97.92%
qiskit/circuit/library/standard_gates/rzx.py 1 97.56%
qiskit/circuit/library/standard_gates/ryy.py 2 95.12%
crates/accelerate/src/synthesis/permutation/utils.rs 2 97.94%
qiskit/circuit/library/standard_gates/rzz.py 2 94.74%
qiskit/circuit/library/standard_gates/p.py 3 96.55%
crates/qasm2/src/lex.rs 5 92.62%
crates/qasm2/src/parse.rs 6 97.61%
qiskit/visualization/circuit/circuit_visualization.py 16 73.43%
Totals Coverage Status
Change from base Build 9718390567: 0.03%
Covered Lines: 64544
Relevant Lines: 71871

💛 - Coveralls

aeddins-ibm and others added 5 commits July 2, 2024 18:40
- fix bugs with checking that ValueError is raised.
- addtionally run all tests on a "flat" data input
We immediately check the lengths of these args, so they should be Sequences, not Iterables.
@aeddins-ibm
Copy link
Contributor Author

aeddins-ibm commented Jul 15, 2024

Before merging and committing to this function signature, should we consider if there are more general postselection conditions we could easily support?

For example, a plausible use case is someone wants all shots where, for all even k, cbits[k] != cbits[k+1].

--
Update: I think this is probably not necessary, at least not urgent. For N values of k, the above example case could be resolved by calling postselect 2N times, which isn't great but is much better than the exponential blowup that I was initially imagining. (I was incorrectly concerned one would have to call postselect 2^N times, once for each of the possible 2N-qubit bitstrings. But one only has to call postselect 2N times, with selection=[0, 1] and selection=[1,0] for each value of k.)

@t-imamichi
Copy link
Member

If you have any use case of cbits[k] != cbits[k+1] in mind, we have a little time left to discuss the extension (before 1.2 release).
If it's not urgent, I agree to merge this PR. This PR is LGTM already.

@aeddins-ibm
Copy link
Contributor Author

aeddins-ibm commented Jul 16, 2024

I think it's OK to merge as-is. The use-case I imagined above can at least be done in a linear number of calls to this function, which seems tolerable for now.

There are other hypothetical use cases that would require an exponential number of calls AFAICT. Like if we wanted only those shots where there are an odd number of 1's (odd parity) in a set of N bits, for large N. Then I think one would need to call postselect an exponential number of times to pick out each of the (2^N)/2 odd-parity bitstrings individually. This is not a fundamental limitation -- postselect could keep all shots where bitwise_xor.reduce of the selected bits gives 1. But I don't see a natural way to generalize the postselect interface, and likewise don't see an obvious place to stop generalizing the logic (if we add xor, do we also need to add xnor? And any? And...).

So I think it's better to leave postselect doing just the one thing it already does.

Someday, if we want to add support for e.g. selecting shots with a certain parity on many bits, we might instead add a separate method BitArray.parity(indices) that returns the parity of the specified bits for each shot, and then the user can use that to create a boolean mask to keep shots with the desired parity. (There may be overlap with code to compute expectation values). This would require modifying slice_shots to accept a boolean mask, which at a glance looks straightforward.

Frankly, we could also use this approach instead of having a postselect method at all, except that currently postselect should be faster than slice_bits due to the bit unpacking/repacking. I was not able to make slice_bits faster via numpy bitwise operations, but maybe a Rust version of slice_bits would be faster, at which point postselect could optionally be deprecated (or not) in favor of doing all types of postselection by creating a boolean mask along the shots axis.

Anyway, that's all to say I agree this PR is good to merge. Thanks for your patient reviewing!

t-imamichi
t-imamichi previously approved these changes Jul 17, 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.

Thank you for your contributions. LGTM.

Post-selection with parity sounds an interesting use case. I agree to think of a separate method to realize it.

@aeddins-ibm
Copy link
Contributor Author

In case it's any use as a future reference, here is one approach to compute parity of specified bits.

This snippet uses bitmask as defined in the postselect code. It first uses bitwise_xor to collapse together all bytes in the bits axis, then broadcasts the remaining 8 bits to a byte each to take their xor.

shot_mask = np.bitwise_xor.reduce(flattened._array & bitmask, axis=-1, dtype=np.uint8, keepdims=True)
shot_mask = np.logical_xor.reduce(shot_mask & (np.uint8(1) << np.arange(8, dtype=np.uint8)), axis=-1, dtype=bool)

@ElePT ElePT added this to the 1.2.0 milestone Jul 22, 2024
aeddins-ibm and others added 2 commits July 23, 2024 09:51
Co-authored-by: Takashi Imamichi <[email protected]>
@t-imamichi t-imamichi requested a review from ihincks July 24, 2024 02:19
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

@t-imamichi
Copy link
Member

Could you review this again, @ihincks? I would like to merge it if there is no problem.

@t-imamichi
Copy link
Member

t-imamichi commented Jul 29, 2024

Since there is no comment from @ihincks, I will merge this PR once CI gets recovered.

@ElePT
Copy link
Contributor

ElePT commented Jul 29, 2024

Neko won't block the merge queue so I think we can already add the PR to the queue :)

@ElePT ElePT added this pull request to the merge queue Jul 29, 2024
@ElePT ElePT added the stable backport potential The bug might be minimal and/or import enough to be port to stable label Jul 29, 2024
Merged via the queue into Qiskit:main with commit 0c03808 Jul 29, 2024
15 checks passed
mergify bot pushed a commit that referenced this pull request Jul 29, 2024
* define BitArray.postselect()

* add test for BitArray.postselect()

* lint

* remove redundant docstring text

* Update qiskit/primitives/containers/bit_array.py

Co-authored-by: Ian Hincks <[email protected]>

* docstring ticks (BitArray.postselect())

Co-authored-by: Ian Hincks <[email protected]>

* Simpler tests for BitArray.postselect

* lint

* add release note

* check postselect() arg lengths match

* fix postselect tests

- fix bugs with checking that ValueError is raised.
- addtionally run all tests on a "flat" data input

* lint

* Fix type-hint

We immediately check the lengths of these args, so they should be Sequences, not Iterables.

* remove spurious print()

* lint

* lint

* use bitwise operations for faster postselect

- Also added support for negative indices
- Also updated tests

* remove spurious print()

* end final line of release note

* try to fix docstring formatting

* fix bitarray test assertion

Co-authored-by: Takashi Imamichi <[email protected]>

* disallow postselect positional kwarg

Co-authored-by: Takashi Imamichi <[email protected]>

* fix numpy dtype args

* Simpler kwarg: "assume_unique"

* lint (line too long)

* simplification: remove assume_unique kwarg

* improve misleading comment

* raise IndexError if indices out of range

- Change ValueError to IndexError.
- Add check for out-of-range negative indices.
- Simplify use of mod
- Update test conditions (include checks for off-by-one errors)

* lint

* add negative-contradiction test

* Update docstring with IndexErrors

* lint

* change slice_bits error from ValueError to IndexError

* update slice_bits test to use IndexError

* change ValueError to IndexError in slice_shots

also update tests for this error

* update error type in slice_shots docstring

* Revert ValueError to IndexError changes

Reverting these changes as they will instead be made in a separate PR.

This reverts commit 8f32178.

Revert "update error type in slice_shots docstring"

This reverts commit 50545ef.

Revert "change ValueError to IndexError in slice_shots"

This reverts commit c4becd9.

Revert "update slice_bits test to use IndexError"

This reverts commit c2b0039.

* fix docstring formatting

Co-authored-by: Takashi Imamichi <[email protected]>

* allow selection to be int instead of bool

* In tests, give selection as type int

* lint

* add example to release note

* fix typo in test case

* add check of test

Co-authored-by: Takashi Imamichi <[email protected]>

* lint

---------

Co-authored-by: Ian Hincks <[email protected]>
Co-authored-by: Takashi Imamichi <[email protected]>
(cherry picked from commit 0c03808)
github-merge-queue bot pushed a commit that referenced this pull request Jul 29, 2024
* define BitArray.postselect()

* add test for BitArray.postselect()

* lint

* remove redundant docstring text

* Update qiskit/primitives/containers/bit_array.py

Co-authored-by: Ian Hincks <[email protected]>

* docstring ticks (BitArray.postselect())

Co-authored-by: Ian Hincks <[email protected]>

* Simpler tests for BitArray.postselect

* lint

* add release note

* check postselect() arg lengths match

* fix postselect tests

- fix bugs with checking that ValueError is raised.
- addtionally run all tests on a "flat" data input

* lint

* Fix type-hint

We immediately check the lengths of these args, so they should be Sequences, not Iterables.

* remove spurious print()

* lint

* lint

* use bitwise operations for faster postselect

- Also added support for negative indices
- Also updated tests

* remove spurious print()

* end final line of release note

* try to fix docstring formatting

* fix bitarray test assertion

Co-authored-by: Takashi Imamichi <[email protected]>

* disallow postselect positional kwarg

Co-authored-by: Takashi Imamichi <[email protected]>

* fix numpy dtype args

* Simpler kwarg: "assume_unique"

* lint (line too long)

* simplification: remove assume_unique kwarg

* improve misleading comment

* raise IndexError if indices out of range

- Change ValueError to IndexError.
- Add check for out-of-range negative indices.
- Simplify use of mod
- Update test conditions (include checks for off-by-one errors)

* lint

* add negative-contradiction test

* Update docstring with IndexErrors

* lint

* change slice_bits error from ValueError to IndexError

* update slice_bits test to use IndexError

* change ValueError to IndexError in slice_shots

also update tests for this error

* update error type in slice_shots docstring

* Revert ValueError to IndexError changes

Reverting these changes as they will instead be made in a separate PR.

This reverts commit 8f32178.

Revert "update error type in slice_shots docstring"

This reverts commit 50545ef.

Revert "change ValueError to IndexError in slice_shots"

This reverts commit c4becd9.

Revert "update slice_bits test to use IndexError"

This reverts commit c2b0039.

* fix docstring formatting

Co-authored-by: Takashi Imamichi <[email protected]>

* allow selection to be int instead of bool

* In tests, give selection as type int

* lint

* add example to release note

* fix typo in test case

* add check of test

Co-authored-by: Takashi Imamichi <[email protected]>

* lint

---------

Co-authored-by: Ian Hincks <[email protected]>
Co-authored-by: Takashi Imamichi <[email protected]>
(cherry picked from commit 0c03808)

Co-authored-by: aeddins-ibm <[email protected]>
Procatv pushed a commit to Procatv/qiskit-terra-catherines that referenced this pull request Aug 1, 2024
* define BitArray.postselect()

* add test for BitArray.postselect()

* lint

* remove redundant docstring text

* Update qiskit/primitives/containers/bit_array.py

Co-authored-by: Ian Hincks <[email protected]>

* docstring ticks (BitArray.postselect())

Co-authored-by: Ian Hincks <[email protected]>

* Simpler tests for BitArray.postselect

* lint

* add release note

* check postselect() arg lengths match

* fix postselect tests

- fix bugs with checking that ValueError is raised.
- addtionally run all tests on a "flat" data input

* lint

* Fix type-hint

We immediately check the lengths of these args, so they should be Sequences, not Iterables.

* remove spurious print()

* lint

* lint

* use bitwise operations for faster postselect

- Also added support for negative indices
- Also updated tests

* remove spurious print()

* end final line of release note

* try to fix docstring formatting

* fix bitarray test assertion

Co-authored-by: Takashi Imamichi <[email protected]>

* disallow postselect positional kwarg

Co-authored-by: Takashi Imamichi <[email protected]>

* fix numpy dtype args

* Simpler kwarg: "assume_unique"

* lint (line too long)

* simplification: remove assume_unique kwarg

* improve misleading comment

* raise IndexError if indices out of range

- Change ValueError to IndexError.
- Add check for out-of-range negative indices.
- Simplify use of mod
- Update test conditions (include checks for off-by-one errors)

* lint

* add negative-contradiction test

* Update docstring with IndexErrors

* lint

* change slice_bits error from ValueError to IndexError

* update slice_bits test to use IndexError

* change ValueError to IndexError in slice_shots

also update tests for this error

* update error type in slice_shots docstring

* Revert ValueError to IndexError changes

Reverting these changes as they will instead be made in a separate PR.

This reverts commit 8f32178.

Revert "update error type in slice_shots docstring"

This reverts commit 50545ef.

Revert "change ValueError to IndexError in slice_shots"

This reverts commit c4becd9.

Revert "update slice_bits test to use IndexError"

This reverts commit c2b0039.

* fix docstring formatting

Co-authored-by: Takashi Imamichi <[email protected]>

* allow selection to be int instead of bool

* In tests, give selection as type int

* lint

* add example to release note

* fix typo in test case

* add check of test

Co-authored-by: Takashi Imamichi <[email protected]>

* lint

---------

Co-authored-by: Ian Hincks <[email protected]>
Co-authored-by: Takashi Imamichi <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Changelog: New Feature Include in the "Added" 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
Status: Done
Development

Successfully merging this pull request may close these issues.

7 participants