feat: Add RXYXY2Q gate - #788
jake-arkinstall wants to merge 3 commits into
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
Batched RPP commands receive noise only on their first pair, and the new decomposition paths lack targeted tests.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Adds RPP as a parameterized two-qubit gate with arbitrary-angle and Clifford simulator support.
Changes:
- Adds RPP metadata, constructors, serialization, display, and dispatch.
- Implements RPP through an RZ–RXX–RZ decomposition.
- Integrates RPP with legacy depolarizing noise models and explicitly rejects it in QASM.
File summaries
| File | Description |
|---|---|
clifford_rotation.rs |
Adds Clifford RPP execution. |
circuit_executor.rs |
Dispatches RPP gates. |
arbitrary_rotation_gateable.rs |
Adds arbitrary-angle decomposition. |
unitary_matrix.rs |
Classifies RPP as parameterized. |
circuit_display.rs |
Adds RPP rendering. |
engine.rs |
Rejects unsupported QASM RPP gates. |
quantum.rs |
Adds engine dispatch paths. |
depolarizing.rs |
Applies two-qubit noise classification. |
biased_depolarizing.rs |
Applies biased two-qubit noise classification. |
builder.rs |
Adds byte-message RPP construction. |
gates.rs |
Adds the RPP gate constructor. |
gate_type.rs |
Defines RPP and its arities. |
Review details
- Files reviewed: 12/12 changed files
- Comments generated: 4
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| | GateType::SZZ | ||
| | GateType::SZZdg | ||
| | GateType::SWAP | ||
| | GateType::RPP |
There was a problem hiding this comment.
This isn't related to this PR - if the review is correct, it'd be the same in all other 2Q gates too and would need addressing separately.
| | GateType::SZZ | ||
| | GateType::SZZdg | ||
| | GateType::SWAP | ||
| | GateType::RPP |
There was a problem hiding this comment.
Same as above.
| /// rotations the same sign. Each ion contributes a minus sign, so the | ||
| /// two-qubit global phase still cancels. | ||
| #[inline] | ||
| fn rpp(&mut self, theta: Angle64, phi: Angle64, pairs: &[(QubitId, QubitId)]) -> &mut Self { |
| /// | ||
| /// # Errors | ||
| /// Returns an error before changing the state if an angle is unsupported. | ||
| fn try_rpp( |
|
RPP is not a great name, .e.g.:
I think a better naming derivation is: XY_φ = cos(φ)X + sin(φ)Y RXY1Q(θ, φ) = exp(-i θ XY_φ / 2) Therefore, to add to confusion, in PECOS I would go with the RXYXY2Q. Ugly... but less confusing. |
|
I have updated the gate name, and I have also added some hooks into Selene where they were previously bailing due to rpp (a.k.a. rxyxy2q) being unsupported. |
ciaranra
left a comment
There was a problem hiding this comment.
How this review was produced
This is an AI-generated review, posted at my request, of commit 8c87d492a. It has not been hand-checked line by line by me, so please push back on anything that looks wrong.
- Primary reviewer: Claude Fable 5.1 (
claude-fable-5-1) running in Claude Code. It read the full diff, swept the repository for gate lists the diff does not touch, compared the gate against Selene's ownrppdefinition, and wrote and ran two throwaway test files in a local worktree of this branch (since deleted, nothing was pushed). - Independent second reviewer: OpenAI
gpt-6-astravia Codex CLI 0.154.0, high reasoning effort, read-only sandbox. It was given the diff and a list of questions, with no access to the first reviewer's conclusions. - Fusion: every Codex finding listed below was re-checked against the code by Claude before being included. Where a claim rests on something that was executed rather than read, it says so.
Summary
The gate itself is correct. The requests are at the edges: one inconsistency this PR introduces in the QASM layer, and two Python paths that silently drop the gate or its noise.
Verified correct
- Definition and Selene convention.
Rz(-phi); RXX(theta); Rz(phi)givesexp(-i theta/2 P(phi) (x) P(phi)). The resulting matrix was compared entry by entry, real and imaginary parts, against the matrix in Selene's QuEST simulator (selene-ext/simulators/quest,rpp). They agree. - Phase exactness beyond the f32 backend. The PR's matrix test uses
StateVecSoA32at 3e-6. A scratch probe ran the same four-column matrix check on the f64 dense state vector, the sparse state vector andStabVec, over a 7x7 grid of angles includingphi = piand negative angles. All agree to 1e-10, so the claim that the two sign flips atphi = picancel holds on those backends. (Executed.) - Clifford path atomicity. The precheck in
try_rxyxy2quses the same snapping astry_rzandtry_rxx, and the Clifford angle grid is closed under negation, so a passing precheck cannot be followed by a failure after the firstrzhas mutated state. - General noise model scales its two-qubit rate by
angles[0], which is theta. That is the right angle. - DEM builder and pecos-neo adapter reject the new gate with an error rather than skipping it.
- Test oracles. The matrix tests derive the expected matrix independently of the implementation; a flipped sign on phi or swapped theta/phi would fail them.
Requested changes
-
QASM accepts the gate at parse time and rejects it at run time.
parse_native_gate(crates/pecos-qasm/src/parser/native_gates.rs:36) now returnsRXYXY2Q, whilecrates/pecos-qasm/src/engine.rs:692returns "not yet supported in the QASM engine". Theis_qasm_native_gatetest helper in the same file classifies the gate as non-native, so the round-trip test skips it and does not notice. Reproduced:RXYXY2Q(pi, 0) q[0], q[1];parsesOkandqasm_sim(...).run(1)returns that error.RXXandRYYare not QASM-native either, so the smallest consistent fix is to remove the parser line. -
TickCircuit.with_noiseapplies no two-qubit noise to the gate.python/pecos-rslib/src/dag_circuit_bindings.rs:2952lists the two-qubit gates explicitly and ends in_ => {}. The gate is already reachable from Python throughadd_gate("RXYXY2Q", ...)becauseGateType::from_straccepts the name. Adding the variant to that arm fixes it. -
Stim export drops the gate without an error.
_gate_to_stiminpython/quantum-pecos/src/pecos/qec/surface/circuit_builder.py:3304falls through toreturn [], None. Codex executed the exporter onRXYXY2Q(pi, 0)withp2 = 1and got an empty circuit. Please either lower the Clifford angles the way theRXY1Qbranch above it does, or raise for this gate. -
_qis_trace_replay.pycannot replay the new operation (both the raw path near line 193 and the runtime-lowered path near line 440). This fails loudly, so it could be a follow-up, but note that traces containing RPP now reach this code where previously the runtime refused them. -
Clifford edge cases (minor).
try_rxyxy2qcomparestheta == Angle64::ZEROexactly, whereastry_rxxsnaps. A theta within snapping tolerance of zero combined with a non-Clifford phi is therefore rejected although the gate is the identity. Snapping theta first, astry_simplify_rxy1qdoes, removes the inconsistency. The Selene stabilizer plugin already does this on its side.theta = piwithphian odd multiple ofpi/4is Clifford ((X+Y)/sqrt(2)swaps X and Y and negates Z), and Selene's Stim simulator accepts it. This PR rejects it. ExistingRXY1Qhas the same limitation, so I do not consider this blocking.
-
docs/user-guide/gates.mdhas no entry for the new gate.
Pre-existing, not caused by this PR
These affect every batched two-qubit gate and should be tracked in separate issues rather than fixed here.
- Copilot's batched-noise comment is correct, and so is the reply that it is not specific to this PR.
apply_tq_faultsincrates/pecos-engines/src/noise/depolarizing.rs:328andbiased_depolarizing.rs:355samples once and applies the fault toqubits[0]andqubits[1]only, so a command carrying several pairs gets noise on the first pair alone. apply_tq_faultsincrates/pecos-engines/src/noise/general.rs:1203evaluateshas_leakageover the whole command inside the per-pair loop, so one leaked qubit removes every pair in the batch, including healthy ones.- Items 2 and 3 above exist because of wildcard arms that silently ignore unknown gate types, the same class as #575.
|
Follow-up to the review above: the two pre-existing batched-noise defects are now tracked separately and do not need to be addressed in this PR.
Both were reproduced by execution with plain |
Adds RXYXY2Q as a GateType, performing the 2-qubit operation: