Standardize executed T/Tdg on the conventional matrix - #601
Conversation
Scope grew during review; here is what changed and whyThe original PR converted only The fix preserves named tokens for the whole phase-sensitive set --
QASM include audit
Two composites depend on the changed primitives:
Neither include contains Callers with pinned absolute amplitudes from QASM Reviews and what they foundThree adversarial review rounds on this branch. Findings that were real and are fixed: the default 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 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: Not verifiedGPU and CUDA paths compile and lint but were never executed -- no hardware here, and CI cannot close this either, since Python's shared |
Reviewer's guideThis diff is 68 files, but the judgement lives in 11 of them. Bucketed by what actually needs thought:
Bucket A, with the specific claim to check in each
Bucket B is repetitive Why this is not split into smaller PRsThe 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
The one thing review cannot settleGPU 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 |
Correction: the GPU paths have now been executed on hardwareMy 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 Re-run on this branch against the real device:
That covers the substantive GPU surface of this PR: the constant tables in What genuinely remains un-executedMuch less than I previously implied. The remainder is 12 changed lines in two optional-dependency Python simulators:
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, nestedThat is 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. |
Execution-level differential: dev vs this branchRather 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: Results.
Phase distribution, in units of pi/8, was identical between the two implementations: What this establishes.
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 |
Closes #597. Also closes #599, whose fix is required here.
Problem
The
T/Tdgtokens executed as different matrices in different parts of PECOS. The CPU state vectors, the GPU constant table and the Python simulators applied the symmetricRZ(pi/4) = diag(e^{-i pi/8}, e^{i pi/8}); cuQuantum and thepecos-quantumnamed-gate matrix table applied the conventionaldiag(1, e^{i pi/4}).SZwas conventional everywhere, soTwas the sole token made symmetric to match its rotation.Consequences:
T . T != SZas executed matrices on the CPU and GPU paths but== SZon cuQuantum, and the same odd-Tcircuit 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.pydefinesTasdiag(1, e^{i pi/4})and asserts equivalence toRZ(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_phasehook defaults to a no-op; the defaultt/tdgapply one symmetricrz(+-pi/4)pluse^{+-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 throughuwas rejected: the trait's defaultuexpands torz.ry.rz, which would have leftTsymmetric for non-overriding implementors and issued three FFI callbacks where the foreign vtable has nory.Named
T/Tdgare now preserved through the whole lowering chain --ByteMessageBuilder, PHIR, the HUGR engine (including theCCXdecomposition'sTcomponents), 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.SparseStateVecSoAkeeps 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 afrac_phasesstep.StabVecmaterialises pending rotations for the target qubits only, so aTon one qubit no longer prunes fused rotations on unrelated ones.Included fix for #599
Angle::half_angle_sin_coshalved the stored unsigned fraction, so for any angle with a negative principal value it returned exactly-1times the correct(sin(theta/2), cos(theta/2)). That is a global-1on the resulting rotation, invisible while comparisons quotiented global phase. It is fixed inpecos-corein 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 aboveHALF_TURNhave a negative principal value but round to exactly pi in f64. Exactly half a turn stays+pi, matchingto_radians_signed's(-pi, pi]range, soRZ(pi)remains-iZ. TheMastcaller was audited: itscorrection_anglecan be negative, so that branch had been applying-RZ(phi).Deliberately unchanged, now documented as projective
RZ(theta),UnitaryRep::T, theRZ(pi/4) -> Tsimplifier rewrite, the neo adaptor lowering, and the Python QASM lowering. These express up-to-global-phase relations, which is what the neighbouringRZ(pi/2) -> SZrewrite already was;Tsimply stops being the odd one out.PyForeignSimulatoris unreachable from any current runner and is documented as projective-only rather than described as executing conventional matrices.Verification
cargo check --workspace --all-targets-- cleancargo test -p pecos-core -p pecos-simulators -p pecos-engines -p pecos-qec-- cleancargo test -p pecos-stab-tn --lib-- 341 passedcargo clippyover the touched crates with-D warnings-- clean, run coldT . T == SZ, exactT^8 == I, oddTandTdgamplitudes, multi-target phase accumulation, and X / Y / Z-only / non-Pauli / mixed-batch frame cases; the two pre-existingT^2 == SZtests now compare components directly instead of quotienting phaseNot 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_vectorshelper still normalises phase. Adding phase-sensitive assertions there is a sensible follow-up, but changing that helper globally would also alter intentionally projective comparisons.