Fix phase of pauli_list.insert(..., qubit=True) for length-1 pauli_list (backport #13624)
#13637
+23
−8
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
![mergify[bot]](https://avatars.githubusercontent.com/in/10562?s=60&v=4){kind=small}
…_list` (#13624) * switch order of if-clauses `len(value) == 1` is the simplest case, and is also the case where the problematic clause `len(value) == size` fails. This commit switches the order, so we check for `len(value) == 1` first. This ensures that when the `len(value) == size` clause runs, we know that `size != 1`, avoiding the bug in #13623. * add test * Simplify `value.phase` broadcasting Here, `phase` should be a 1D array. `np.vstack()` docs say explicitly output will be at least 2D, so we should not use that to create `phase`. The intent of using `np.vstack()` was essentially to broadcast `phase` to properly add to `self.phase`. But, this happens automatically and correctly if we just add as-is. So this commit simplifies the code accordingly. * Verify that `phase` is 1D in `from_symplectic()` Verifying the input arg `phase`, since otherwise `from_symplectic()` will silently create PauliLists with malformed `phase` attributes (i.e. not 1D). Zero-dimensional is OK (e.g. `phase` defaults to `0`) since that can broadcast per the shape of the `z` and `x` arrays. * remove vestigial line * lint * release note * Update releasenotes/notes/fix-paulilist-length1-phase-688d0e3a64ec9a9f.yaml Co-authored-by: Jake Lishman <[email protected]> --------- Co-authored-by: Jake Lishman <[email protected]> (cherry picked from commit 408741c)
|
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:
|
github-actions
bot
added
Changelog: Bugfix
Pull Request Test Coverage Report for Build 12677497573Details
💛 - Coveralls |
jakelishman
approved these changes
Jan 8, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
Fix #13623.
Details and comments
Fixes bug in
PauliList.insert(..., qubit=True)method, where the outputPauliListwould have a 2Dphaseattribute (should be 1D).The bug was in a line of code manually broadcasting the
phaseof thePauliListbeing inserted to match the shape of thephaseof the originalPauliList. This PR removes this line of code, and lets numpy handle the broadcasting ofphase.This PR also adds a check in
PauliList.from_symplectic(..., phase)to rejectphaseif it is a multi-dimensional array, and a test for issue #13623.(For the record, I messed up the commit message in 92161ec. The simpler
if/elifcase is indeed moved to be first, but in the commit message I mixed up the two cases).This is an automatic backport of pull request #13624 done by [Mergify](https://mergify.com).