fix(gate-decompose): correct the sign on the first RZ of the RX sequence - #4
Merged
Merged
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.
rustnew
added a commit
that referenced
this pull request
Aug 25, 2026
…aining it Fixes the other half of #3 (the RX sign was already fixed by #4). The pass built a native decomposition chain, inserted it before the original op, and re-pointed the original op's *inputs* to the chain's output — but the original op itself stayed in the block and still produced its own result, which every downstream consumer kept using. So decomposing T didn't replace it: it composed Rz(pi/4) with the still-present T, which is S, not T's actual decomposition. Same for every other entry in the table. Now the original op's results are redirected to the decomposition chain's final results (scanning all ops' inputs, same pattern gate-cancellation already uses for its own redirects), and the original op is deleted from both its block and the ops slotmap. Strengthened test_ibm_decomposes_h to assert H is actually gone (it previously only checked the decomposition's gates were present, which passed whether or not H survived), and added test_original_gate_is_removed_not_chained. 539/539 tests, examples/validate_all.sh 105/105, clippy -D warnings and fmt --check clean. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
rustnew
added a commit
that referenced
this pull request
Aug 25, 2026
Several code examples used APIs that don't exist and would not compile: - ctx.type_interner -> ctx.types (real field name) - lift_quantum::dialect::Provider -> lift_quantum::gates::Provider (wrong module path) - Provider::Ibm -> Provider::IbmEagle/IbmKyoto/etc. (no bare Ibm variant) - GateDecomposition::new(Some(Provider::Ibm)) -> the real signature takes Provider directly, not Option<Provider> (2 occurrences) - tensor_info.size_bytes() doesn't exist on TensorTypeInfo; replaced with the real element-count x dtype-size computation Other fixes, all verified against source and/or the CLI: - "tensor/quantum/hybrid dialects registered automatically" was false — only core is automatic via Context::new(); the others need explicit register_*_dialect() calls (this is what lift-cli's verify command does). Replaced the claim with a working example. - LayoutMapping was described as inserting SWAP gates in two places; it's annotation-only (needs_swap = true) — RealRouting does the actual insertion. Fixed both, and updated §5.2's "Combine with" pointer. - The O0-O3 pass table (§12.3) contradicted both §7.4 and the real OptimisationConfig::passes_for_level: O1 was missing constant-folding, O2/O3 were wrong about which passes they add. Rewritten to match. - The RX decomposition comment still showed the pre-fix buggy sign (RZ(-pi/2)...) from before this session's #4 fix landed. - The OpenQASM export note repeated the "10/50+ gates supported" claim that turned out to be false everywhere it appeared this session (see the CAPABILITIES.md/LIFT_design.md fix) — corrected to all 48. - 3 tensor ops (Clamp, SparseEmbedding, Where) and 2 quantum gates (GPI, GPI2) were missing from the "110 ops"/"48 gates" reference tables, and the ops table's numbering had a pre-existing bug (jumped 106->110). Added the 5 missing entries and renumbered the whole table 1-110 against the real TensorOp enum, zero diff. Verified working via the CLI: the MLP example, the Bell-state example, and both .lith config examples all pass verify/optimise as shown. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
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
2 participants
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.