Fix sx resynthesis of negative angles in CommutativeCancellation#16599
Conversation
`%` maintains the sign of the left operand, so `0..(num_sx as i64 % 4)` is an empty iterator for non-positive `num_sx`. If the calculated rotation angle was negative (such as from a chain of `sxdg`), non-identity, and we go down the `sx`-like synthesis path, we would miscompile. This is a minimal fix to be backport stable, rather than a greater reworking. Introduced in 91081cc: Add additional Z and X rotation gate handling to CommutativeCancellation (Qiskit#16191)
jakelishman
added
stable backport potential
|
One or more of the following people are relevant to this code:
|
alexanderivrii
left a comment
alexanderivrii
left a comment
There was a problem hiding this comment.
Thanks for the fix! One super-minor comment inline.
| def test_negative_x_rotations(self): | ||
| qcs, expecteds = [], [] | ||
| for num_sxdg in (2, 3, 4): | ||
| qc = QuantumCircuit(1) | ||
| for _ in range(num_sxdg): | ||
| qc.sxdg(0) | ||
| qcs.append(qc) | ||
| expected = qc.copy_empty_like() | ||
| for _ in range(4 - num_sxdg): | ||
| expected.sx(0) | ||
| expecteds.append(expected) | ||
| pass_ = CommutativeCancellation(["sx", "rz"]) | ||
| self.assertEqual([pass_(qc) for qc in qcs], expecteds) |
There was a problem hiding this comment.
Nitpick: it might be nice to split this into separate tests using @ddt.data. That said, I don't feel strongly about it and am happy to merge as is.
|
Thanks for the quick fix! |
|
Shelly: sounds like it's worth a quick scan, sure. |
4 participants
%maintains the sign of the left operand, so0..(num_sx as i64 % 4)is an empty iterator for non-positivenum_sx. If the calculated rotation angle was negative (such as from a chain ofsxdg), non-identity, and we go down thesx-like synthesis path, we would miscompile.This is a minimal fix to be backport stable, rather than a greater reworking.
Introduced in 91081cc: Add additional Z and X rotation gate handling to CommutativeCancellation (#16191)
Fix #16594
AI/LLM disclosure