Skip to content

Standardize executed T/Tdg on the conventional matrix - #601

Open
ciaranra wants to merge 4 commits into
devfrom
t-matrix-convention
Open

Standardize executed T/Tdg on the conventional matrix#601
ciaranra wants to merge 4 commits into
devfrom
t-matrix-convention

Conversation

@ciaranra

Copy link
Copy Markdown
Member

Closes #597. Also closes #599, whose fix is required here.

Problem

The T / Tdg tokens executed as different matrices in different parts of PECOS. The CPU state vectors, the GPU constant table and the Python simulators applied the symmetric RZ(pi/4) = diag(e^{-i pi/8}, e^{i pi/8}); cuQuantum and the pecos-quantum named-gate matrix table applied the conventional diag(1, e^{i pi/4}). SZ was conventional everywhere, so T was the sole token made symmetric to match its rotation.

Consequences: T . T != SZ as executed matrices on the CPU and GPU paths but == SZ on cuQuantum, and the same odd-T circuit produced state vectors differing by a global phase depending on which simulator ran it.

PECOS's own reference oracle already took the conventional side -- python/quantum-pecos/tests/pecos/integration/state_sim_tests/gate_matrix_def.py defines T as diag(1, e^{i pi/4}) and asserts equivalence to RZ(pi/4) only up to phase. This change makes the code agree with the oracle it already ships.

Approach

The executed token matrix is standardised on the conventional form. RZ(theta) remains the symmetric rotation.

A new ArbitraryRotationGateable::apply_global_phase hook defaults to a no-op; the default t / tdg apply one symmetric rz(+-pi/4) plus e^{+-i pi/8} per target. Simulators exposing absolute amplitudes implement the hook or an explicit conventional kernel; representations where a global phase is unobservable (density matrices, the measurement-only mock, the FFI facade, the compile-only tableau) inherit the no-op. Routing through u was rejected: the trait's default u expands to rz.ry.rz, which would have left T symmetric for non-overriding implementors and issued three FFI callbacks where the foreign vtable has no ry.

Named T / Tdg are now preserved through the whole lowering chain -- ByteMessageBuilder, PHIR, the HUGR engine (including the CCX decomposition's T components), HUGR-to-QIS, and the QIS dialect, parser and converter -- so the conventional kernel is reached by real execution paths and not only by direct simulator calls.

SparseStateVecSoA keeps its live Pauli frame and records the commutation scalar: X T X = Y T Y = e^{i pi/4} T-dagger, so a live X or Y frame applies the opposite kernel and records a frac_phases step. StabVec materialises pending rotations for the target qubits only, so a T on one qubit no longer prunes fused rotations on unrelated ones.

Included fix for #599

Angle::half_angle_sin_cos halved the stored unsigned fraction, so for any angle with a negative principal value it returned exactly -1 times the correct (sin(theta/2), cos(theta/2)). That is a global -1 on the resulting rotation, invisible while comparisons quotiented global phase. It is fixed in pecos-core in fixed point rather than at a call site, with the sign test made on the fraction rather than a converted f64: near half a turn that is strictly more accurate, since fractions one unit above HALF_TURN have a negative principal value but round to exactly pi in f64. Exactly half a turn stays +pi, matching to_radians_signed's (-pi, pi] range, so RZ(pi) remains -iZ. The Mast caller was audited: its correction_angle can be negative, so that branch had been applying -RZ(phi).

Deliberately unchanged, now documented as projective

RZ(theta), UnitaryRep::T, the RZ(pi/4) -> T simplifier rewrite, the neo adaptor lowering, and the Python QASM lowering. These express up-to-global-phase relations, which is what the neighbouring RZ(pi/2) -> SZ rewrite already was; T simply stops being the odd one out.

PyForeignSimulator is unreachable from any current runner and is documented as projective-only rather than described as executing conventional matrices.

Verification

  • cargo check --workspace --all-targets -- clean
  • cargo test -p pecos-core -p pecos-simulators -p pecos-engines -p pecos-qec -- clean
  • cargo test -p pecos-stab-tn --lib -- 341 passed
  • cargo clippy over the touched crates with -D warnings -- clean, run cold
  • Rust doc-test crate -- clean
  • New tests assert exact T . T == SZ, exact T^8 == I, odd T and Tdg amplitudes, multi-target phase accumulation, and X / Y / Z-only / non-Pauli / mixed-batch frame cases; the two pre-existing T^2 == SZ tests now compare components directly instead of quotienting phase
  • Guards were mutation-tested: removing the phase hook, or reverting the half-turn boundary, fails the intended tests

Not verified here

GPU and CUDA paths compile and lint but were not executed -- no hardware GPU or CUDA driver in this environment. They were reviewed by inspection and are mutually consistent with the CPU result.

Python's shared _compare_vectors helper still normalises phase. Adding phase-sensitive assertions there is a sensible follow-up, but changing that helper globally would also alter intentionally projective comparisons.

@ciaranra

Copy link
Copy Markdown
Member Author

Scope grew during review; here is what changed and why

The original PR converted only T/Tdg. A pre-merge review found that this made things worse than dev in the production execution path: dev was uniformly projective and therefore self-consistent, whereas converting T alone left ByteMessageBuilder::sz still emitting RZ(pi/2), so executed T . T and executed named S differed by e^{i pi/4}. The exact identity this PR exists to establish was false where it mattered most.

The fix preserves named tokens for the whole phase-sensitive set -- S, Sdg, T, Tdg, SX, SXdg -- through every lowering path to a simulator: byte-message, PHIR, both QIS collection routes, HUGR (including the CCX decomposition), HUGR-to-QIS, the QIS dialect/parser/converter, the Rust QASM includes and engine, and the experimental StabMps/Mast Python bindings, which were still dispatching "T" to RZ(pi/4) and bypassing the new hook while exposing amplitudes.

byte_message_engine_executes_t_squared_as_exact_s now asserts the identity at engine level.

QASM include audit

qelib1.inc and hqslib1.inc now define s, sdg, t, tdg, sx, sxdg as native tokens rather than rotation expansions. Each substitution changes only a global phase, and each new definition matches the OpenQASM standard library exactly -- the standard defines s as diag(1, i), not a symmetric rotation, so this brings PECOS into agreement with it. The standard has no named sxdg; PECOS's is the exact adjoint of standard SX.

Two composites depend on the changed primitives:

  • cy -- its sdg and s phases cancel exactly. Both old and new bodies implement exact CY.
  • ccx -- four t and three tdg, so new/old is e^{i pi/8}. The new body is exact CCX; the old body was e^{-i pi/8} CCX. This change incidentally fixes that.

Neither include contains ctrl @ or a conditional, so no changed primitive sits beneath coherent control and no global phase becomes an observable relative phase.

Callers with pinned absolute amplitudes from QASM s/t/sx will see a global-phase change; callers comparing probabilities will not. Worth a release note.

Reviews and what they found

Three adversarial review rounds on this branch. Findings that were real and are fixed: the default t routed through u (whose default expands to rz.ry.rz, leaving T symmetric for non-overriding implementors and issuing an ry callback the foreign vtable does not have); the engine path converting named T to RZ before reaching any simulator; StabVec flushing the whole register instead of the target qubits; PyForeignSimulator inheriting the no-op hook without justification; and the S/Sdg gap above.

One reported blocker was investigated and is not part of this PR: both density-matrix implementations apply the system operation to the traced purification index and the conjugate to the physical row index, so a T token evolves rho as T^dagger rho T. git diff origin/dev for both density files is empty on this branch -- it is pre-existing, and the GPU-versus-CPU test cannot see it because both share the reversal. Filed as #607 rather than folded in here.

The final review round on the newly added surface returned no must-fix finding. Its two follow-ups were closed in this branch rather than deferred: HugrDialect now registers the full named-gate set, and the converter reads the same shared definition table so acceptance and registration cannot drift apart; a validation test fails if a future gate is accepted by the converter but left unregistered.

Not verified

GPU and CUDA paths compile and lint but were never executed -- no hardware here, and CI cannot close this either, since cuda-build-check is cargo check in stub mode on ubuntu-latest. They were reviewed by inspection and are mutually consistent with the CPU result.

Python's shared _compare_vectors helper still normalises global phase. Adding phase-sensitive assertions there is a reasonable follow-up, but changing that helper wholesale would also alter intentionally projective comparisons.

@ciaranra

Copy link
Copy Markdown
Member Author

Reviewer's guide

This diff is 68 files, but the judgement lives in 11 of them. Bucketed by what actually needs thought:

bucket files lines
A -- real semantics, read closely 11 831
B -- token plumbing, skim one and you have seen them all 18 891
C -- tests 15 559
D -- docs and comments 24 106

Bucket A, with the specific claim to check in each

  • crates/pecos-simulators/src/state_vec_sparse_soa.rs (227) -- the subtlest thing here. These kernels do not flush a live Pauli frame; they push the gate through it. That was phase-free for the symmetric rotation but is not for the conventional gate: X T X = Y T Y = e^{i pi/4} T^dagger. The claim to check is that the recorded frame_phases step matches that algebra and its sign convention agrees with the neighbouring cz / szz / iswap paths.
  • crates/pecos-simulators/src/arbitrary_rotation_gateable.rs (107) -- the apply_global_phase hook. Default is a no-op, which is correct exactly where a global phase is unobservable. The claim to check is that the no-op is justified for every implementor that inherits it, and that the default t still emits one operation rather than three.
  • exp/pecos-stab-tn/src/stab_mps.rs (119) and stab_mps/mast.rs (18) -- global-phase tracking, accumulated once per target qubit.
  • crates/pecos-simulators/src/stab_vec.rs (55) -- materialises pending rotations for the target qubits only. An earlier revision flushed the whole register, which changed results on unrelated qubits under default pruning.
  • crates/pecos-phir/src/hugr_parser.rs (111) -- preserves rotations at Clifford angles. Now that named S is phase-sensitive, silently rewriting a source RZ(pi/2) into a named S would change a projective rotation into a phase-sensitive gate.
  • crates/pecos-core/src/angle/trig.rs (102) -- this is Angle::half_angle_sin_cos returns -sin(theta/2), -cos(theta/2) for negative angles #599, and is identical to PR Return the signed half-angle from Angle::half_angle_sin_cos for negative angles #604. If Return the signed half-angle from Angle::half_angle_sin_cos for negative angles #604 lands first, rebasing drops this file from the diff entirely.
  • crates/pecos-engines/src/byte_message/builder.rs (48) -- named-token encoding. This is the file whose half-done state caused production T^2 != S.
  • crates/pecos-qasm/includes/qelib1.inc (24) and hqslib1.inc (18) -- see the audit in the previous comment. Each substitution changes only a global phase, each new definition matches the OpenQASM standard, and ccx becomes exact where it previously carried e^{-i pi/8}.

Bucket B is repetitive T/Tdg/S/Sdg/SX/SXdg named-token plumbing through PHIR, QIS, HUGR and QASM. Skimming one is representative.

Why this is not split into smaller PRs

The simulator-kernel change and the lowering-path change are not independently safe. Converting the executed token without preserving it through lowering is exactly the state that made production T^2 != S mid-review; preserving tokens without converting the kernels does nothing. They have to land together or the tree is inconsistent.

angle/trig.rs genuinely is separable, and is separated -- as #604.

The one thing review cannot settle

GPU and CUDA paths compile and lint but were never executed: there is no hardware in the environment used to prepare this, and CI cannot close it either, since cuda-build-check is cargo check in stub mode on ubuntu-latest. Those diffs are small constant tables and dispatch arms, reviewed by inspection and consistent with the CPU result, but nobody has run them.

@ciaranra

Copy link
Copy Markdown
Member Author

Correction: the GPU paths have now been executed on hardware

My earlier comment said the GPU paths were never executed and that no hardware was available. That was wrong, and I want to correct it rather than leave it standing.

The machine has an NVIDIA GeForce RTX 4090 (driver 580.173.02). The earlier failures came from a sandboxed tool process that could not reach /dev/nvidia* and /dev/dri, and I incorrectly generalised that to the whole environment.

Re-run on this branch against the real device:

GpuAdapterInfo { name: "NVIDIA GeForce RTX 4090", backend: Vulkan, device_type: DiscreteGpu }
  • cargo test -p pecos-gpu-sims --lib -- 193 passed, 0 failed
  • cargo test -p pecos-gpu-sims --test gate_fuzz -- 5 passed, 0 failed. This is the CPU-versus-GPU componentwise amplitude comparison, and its generated circuits include T and Tdg, so it is exactly the test that would catch a wrong GPU T matrix. It is also the test that previously failed with Device type Cpu is not a hardware GPU, so its passing is itself evidence that a discrete GPU was used rather than a software fallback.
  • cargo test -p pecos-gpu-sims across all targets -- all green
  • GPU density-matrix tests -- 14 passed

That covers the substantive GPU surface of this PR: the constant tables in lib.rs, gpu.rs, gpu64.rs, the auto-dispatch in gpu_auto.rs, and gpu_density_matrix.rs.

What genuinely remains un-executed

Much less than I previously implied. crates/pecos-cuquantum/ is not modified by this PR at all -- cuQuantum was already on the conventional convention, and was one of the two authorities the CPU paths were brought into line with.

The remainder is 12 changed lines in two optional-dependency Python simulators:

  • python/quantum-pecos/src/pecos/simulators/custatevec/gates_one_qubit.py (+8 / -2) -- needs cuquantum, not installed here
  • python/quantum-pecos/src/pecos/simulators/mps_pytket/gates_one_qubit.py (+8 / -2) -- needs pytket, not installed here

Both changes are literal matrix definitions and fully checkable by reading:

phase  = cmath.exp(+1j * pi/4)      # T
matrix = [1, 0, 0, phase]           # cuStateVec, row-major 2x2
matrix = [[1, 0], [0, phase]]       # MPS, nested

That is diag(1, e^{+i pi/4}) for T and diag(1, e^{-i pi/4}) for Tdg, in each backend's expected layout -- the conventional matrices, matching the CPU kernels.

I have filed #608 about CI having no GPU runner, which remains true and is worth fixing independently; it just is not a blocker for reviewing this PR.

@ciaranra

Copy link
Copy Markdown
Member Author

Execution-level differential: dev vs this branch

Rather than argue the change file by file, I ran it. Both trees were built and driven through an identical seeded corpus, then the resulting state vectors were compared numerically.

Corpus. 200 seeded random circuits, 5 qubits, 30 gates each, drawn from exactly the tokens whose executed matrices this PR changes plus their neighbours: h, t, tdg, sz, szdg, x, y, z, sx, cx. The same corpus was run through two independent implementations -- StateVecSoA, which reaches the conventional matrix through the new apply_global_phase hook, and SparseStateVecSoA, which uses the hand-written frame-aware kernels. 400 circuit runs per tree.

Results.

dense (hook path) sparse (frame-aware kernels)
circuits 200 200
probability mismatches 0 0
worst |dP| 4.44e-16 8.88e-16
amplitude diffs that are NOT a single global phase 0 0
circuits with no change at all 30 30
every global phase a multiple of pi/8 yes yes

Phase distribution, in units of pi/8, was identical between the two implementations: {-6:1, -5:3, -4:11, -3:13, -2:28, -1:31, 0:30, 1:22, 2:26, 3:17, 4:12, 5:2, 6:2, 7:2}.

What this establishes.

  1. Nothing observable changed. Probabilities agree to within 8.9e-16, which is floating-point noise at machine epsilon, not a semantic difference. Any measurement-based result, DEM, or expectation value is unaffected.
  2. Every difference is a single global scalar. For each circuit, the ratio between the new and old amplitude is constant across the entire state vector. Nothing became a relative phase anywhere in the corpus.
  3. The phase quantum is exactly pi/8, which is what the convention predicts and nothing else: each T/Tdg contributes +-pi/8 and each S/Sdg/SX contributes +-pi/4. A stray factor from some other source would have shown up as a non-multiple.
  4. The two mechanisms agree exactly. The hook path and the explicit frame-aware kernels produce the same phase for every circuit, so they are not merely each self-consistent -- they are consistent with each other.

The harness was temporary and is not part of this branch; both working trees are clean.

Combined with the GPU suites now having been executed on real hardware (RTX 4090, including the gate_fuzz componentwise CPU-versus-GPU amplitude comparison over T/Tdg), the remaining un-executed surface is 12 lines in two optional-dependency Python simulators, which are literal diag(1, e^{+-i pi/4}) matrix definitions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

1 participant