-
Notifications
You must be signed in to change notification settings - Fork 615
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
Update condition for swapping iterators in PauliWord._matmul
#5301
Conversation
Hello. You may have forgotten to update the changelog!
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks @mudit2812
Perhaps add a test ensuring that behavior
H = qml.prod(X(0), X(1), X(2))
Sum_coeffs, Sum_ops = H.terms()
assert Sum_ops[0].wires == H.wires
or update one of the existing terms()
tests to also check the wires
Also changelog/bugfixes
[sc-57962] |
I wouldn't call this a bug fix, as the repr wasn't incorrect, and it wasn't impacting any behaviour. I'll add tests and re-request review |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #5301 +/- ##
==========================================
- Coverage 99.69% 99.68% -0.01%
==========================================
Files 410 410
Lines 38230 37939 -291
==========================================
- Hits 38113 37821 -292
- Misses 117 118 +1 ☔ View full report in Codecov by Sentry. |
…o pauli-swap-order
…o pauli-swap-order
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Love it, thanks so much @mudit2812 ❤️
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for fixing this! I remember being bugged by this when we were working on the ham-tests
branch. 🎸
Hey folks, since we have sufficient approvals can this be merged, or is there more work coming? If more work, is there a test or two we can add to demonstrate the changes? |
No other work @mlxd . I was just blocked by reviews on the related PR in lightning. I wanted to have that one approved before merging this so that we can push the appropriate fix to lightning ASAP after this gets merged. |
Context:
We found that the wire order in
Sum/Prod.terms
was being swapped for the first and second term in terms that wereProd
s. This was due to the use of thepauli_rep
. While building the pauli rep, we usePauliWord._matmul
which swaps the order ofself
andother
iflen(other) >= len(self)
so that we iterate through the largerPauliWord
.Example:
>>> Sum_ops[0].wires, H.wires (<Wires = [1, 0, 2]>, <Wires = [0, 1, 2]>)
Description of the Change:
Changed the condition for swapping from
len(other) >= len(self)
tolen(other) > len(self)
.Benefits:
More consistent
Sum/Prod.pauli_rep
, andSum/Prod.terms()
with the original operands. The above example now behaves as follows:>>> Sum_ops[0].wires, H.wires (<Wires = [0, 1, 2]>, <Wires = [0, 1, 2]>)
Possible Drawbacks:
Related GitHub Issues: