fix(gate-decompose): correct the sign on the first RZ of the RX sequence - #4
Open
cleitonaugusto wants to merge 2 commits into
Open
fix(gate-decompose): correct the sign on the first RZ of the RX sequence#4cleitonaugusto wants to merge 2 commits into
cleitonaugusto wants to merge 2 commits into
Conversation
The RX arm emitted rz(-pi/2), sx, rz(pi + theta), sx, rz(pi/2), which does not implement RX(theta) for any angle. The clearest case is theta = 0: the identity compiled to diag(1, -1), which is Z. Changing the first rotation to rz(+pi/2) makes the sequence equal RX(theta) up to global phase. Adds test_rx_decomposition_matches_rx, which builds the operator from whatever the table returns and compares it against RX(theta) at eight angles, so it keeps checking the real entry rather than a copy. The test fails on the old sign and passes on the new one.
|
Important Review available on request
Reviews should be triggered manually for repositories with fewer than 10 stars. Select Trigger review above or comment ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
…order The first version of this branch failed both CI gates: rustfmt reformatted the matrix literals, and clippy rejected map_or under -D warnings. Both are fixed here, and I ran fmt, clippy and the workspace suite locally this time. The ordering comment in the rx test was also claiming something the test could not check. Every entry in the decomposition table is a single gate, a palindrome or a conjugation, so the product is the same in either order and no table entry can pin the convention down. test_composition_is_in_circuit_order does it directly, with an asymmetric pair and an expected matrix written out by hand rather than composed. That second part matters: comparing mmul against mmul passes even when mmul itself is inverted, since the error cancels on both sides. With the literal matrix the test fails when the multiplication order is flipped. Also loosens the phase comparison to be relative to the entry size, since an absolute bound fails on correct code when a matrix entry is near zero, and names the sx convention the test assumes.
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Fixes the RX half of #3.
The RX arm of the decomposition table emits, in circuit order:
That does not implement RX(theta). I compared it against RX(theta) up to global
phase at theta in {0, 0.3, 1.0, pi/2, pi, 2.2, -0.7, 3.9} and it fails at all of
them. The clearest case is theta = 0, which should be the identity and instead
comes out as
which is Z.
Changing the first rotation to rz(+pi/2) makes the sequence equal RX(theta) up
to global phase at every angle I tried.
The test I added builds the operator from whatever the table returns rather than
from a copy of the sequence, so it keeps checking the real entry if the table
changes later. It fails on the old sign and passes on the new one. I ran the
whole workspace suite and nothing else moved.
The comment above that arm gave two different formulas, one with six gates and
one with five. I replaced it with the sequence that is actually emitted.
This is only the sign. The other half of #3, that gate-decomposition never
removes the original op, is untouched here since it needs a different change.
Happy to send that one too if you want it.