fix(tableau): correlated-loss convention, duplicate batch targets, CZ-block overlap by Roger-luo · Pull Request #205 · QuEraComputing/ppvm

Roger-luo · 2026-08-12T00:43:09Z

Three defects in the crates the ppvm wheel actually ships. All three are user-visible today through the Python API. Found by auditing the Lean formalization in lean/ against the Rust, then adjudicating each with an independent re-derivation before any code changed.

Scoped to the legacy crates deliberately, so the diff stays reviewable. The ppvm-*-2 crates have the same three fixes on codex/traits-2-impl, but ppvm-python-native binds legacy only (cargo tree -p ppvm-python-native contains zero -2 crates), so nothing there reaches a user.

1. Correlated loss disagreed between backends by a factor of two

p[1] is the probability that a named one of the pair is lost, so P(exactly one lost) is 2·p[1] and the survivor scales by 1 − 2·p[1] − p[0]. That is what the paper draft specifies (§"Correlated loss": "the probability that both qubits remain … is $1-p_{LL}-2p_{LQ}$"), and what ppvm-pauli-sum has always computed — its own test comment said (1 - 2*p[1] - p[0]) when the channel first landed in #38.

ppvm-tableau's trajectory and ppvm-tableau-sum's mixture instead read the trait's ambiguous "the probability of losing either one qubit" as the total, giving p[1]. So the same channel gave different answers depending on the backend:

`p`	`LossyPauliSum`	`GeneralizedTableau`	`2·p[1]`
`[0.0, 0.3, 0.0]`	0.600000	0.298450	0.60	before
`[0.1, 0.2, 0.0]`	0.400000	0.197000	0.40	before
`[0.2, 0.4, 0.0]`	0.800000	0.399300	0.80	before

After, all three backends agree with 2·p[1] across the admissible region, including the saturating boundary p[0] + 2·p[1] == 1:

`p`	PauliSum	Trajectory	Mixture	`2·p[1]`
`[0.0, 0.3, 0.0]`	0.600000	0.598650	0.598300	0.60
`[0.2, 0.4, 0.0]`	0.800000	0.800650	0.801600	0.80
`[0.0, 0.5, 0.0]`	1.000000	1.000000	1.000000	1.00

The wording is the actual root cause, so it is now stated normatively in exactly one place — ppvm-traits' CorrelatedLossChannel — and every other site cites it: both tableau backends, ppvm-pauli-sum, mixins.py, paulisum.py (whose "losing a single qubit" was the ambiguity that let the split through), and the usage skill, which mislabelled the triple as a Pauli-error vector [p_x, p_y, p_z] rather than [p_LL, p_LQ, p_LN].

Worth flagging for reviewers: this exact fix was proposed in review on #38 and not applied. A reviewer asked for p[1] to be documented as "per outcome … the total probability of a single-qubit loss event is 2 * p[1]" plus the constraint 2*p[1] + p[0] <= 1. Neither landed, the ambiguous wording shipped, and #34 three weeks later implemented the other reading against it.

Also adds debug_asserts for the admissible region p[0], p[1] >= 0, p[0] + 2·p[1] <= 1, p[2] ∈ [0, 1], with 1e-9 slack so a saturated triple like [1/3, 1/3, _] isn't rejected by rounding. Tests, benches and Python tests that passed out-of-domain triples are corrected to admissible ones without changing a single assertion — e.g. [0.0, 1.0, 0.0], which describes a map that is not completely positive, becomes [0.0, 0.5, 0.0]: the same "exactly one lost every shot" witness, inside the domain.

2. Duplicate qubit indices silently collapsed in batched Cliffords

build_masks ORs one bit per target, so a repeated index applied the gate once instead of k times. X 0 0 is legal Stim meaning apply-per-target, so:

ppvm_stim::run_string("X 0 0\nM 0")// returned Some(true); truth is Some(false)

Now detected with a popcount against the index count, falling back to the per-index loop, which conjugates by G^k correctly for every gate family. XOR-cancelling the mask would be wrong for s/sqrt_x/sqrt_y, where S² = Z ≠ I.

The fallback bodies are #[cold] #[inline(never)] so the fused sweeps keep their register budget — the ten bit-plane batch rows measure 0.990–1.008× against origin/main.

3. The fused CZ block corrupted state when pairs overlapped

cz_block/cz_block_pairs assumed disjoint support — which the Lean proves is necessary, but nothing enforced. On X₀X₁X₂, cz_block(0, 1, 2) returned +Y₀Y₁Y₂ where the per-pair loop gives −Y₀X₁Y₂: wrong in both bit planes and in the sign. cz_block(0, 1, n) is adjacent-pair brickwork, so the natural call was the broken one.

Now falls back per pair when offset < count. Disjoint calls stay bit-for-bit on the fused kernel.

Verification

cargo test --workspace — 65 targets, zero failures
pytest ppvm-python/test/ — 217 passed against a rebuilt wheel (staleness of the installed _core was checked first, so this isn't a false green)
cargo fmt --all --check clean; the cargo clippy pre-commit hook passes. The four workspace-wide --all-targets clippy diagnostics are pre-existing — verified byte-identical against a pristine origin/main checkout, none anchored in a line this PR wrote
Performance: interleaved A/B against a pristine origin/main checkout, 35 rows, 0.985–1.021×, all inside the noise floor. Deliberately includes the five bit-plane gates and the untouched cz/cnot/scalar rows rather than only phase-only gates
cargo bench builds again (it was panicking on an out-of-domain triple)

Known follow-ups, deliberately not in this PR

ppvm-pauli-sum's impl is coefficient-generic and Coefficient carries no ordering, so it documents the admissible region but cannot assert it — it still produces negative coefficients where the tableau backends now raise.
The guards are debug_asserts, so release wheels gain no protection, and a maturin develop install raises PanicException rather than a ValueError from the binding layer.
Unrelated pre-existing bug found while building the verification harness: GeneralizedTableauSum(sum_cutoff=0.0) panics on sampling because branch weights 0.5+0.2+0.2+0.1 sum to 0.9999999999999999, tripping a >= 1 - sum_cutoff assert at tableau-sum/src/data.rs:136. Convention-independent — the old weights round identically — and reachable from Python.

🤖 Generated with Claude Code

…-block overlap Three defects in the crates the `ppvm` wheel actually ships, found by auditing the Lean formalization against the Rust and adjudicated with independent re-derivation. Each is user-visible today through the Python API. **1. Correlated loss disagreed between backends by a factor of two.** `p[1]` is the probability that a *named* one of the pair is lost, so P(exactly one lost) is `2·p[1]` and the survivor scales by `1 − 2·p[1] − p[0]`. That is what the paper specifies, and what `ppvm-pauli-sum` has always computed — its own test comment said `(1 - 2*p[1] - p[0])` when the channel landed. `ppvm-tableau`'s trajectory and `ppvm-tableau-sum`'s mixture instead read the trait's ambiguous "losing either one qubit" as the *total*, giving `p[1]`. So a user got one answer from `LossyPauliSum` and half of it from `GeneralizedTableau`: p LossyPauliSum GeneralizedTableau 2*p[1] [0.0, 0.3, 0.0] 0.600000 0.298450 0.60 before [0.2, 0.4, 0.0] 0.800000 0.399300 0.80 before All three backends now agree with `2·p[1]` across the admissible region, including the saturating boundary `p[0] + 2·p[1] == 1`. The wording is the actual root cause, so it is now stated normatively once, on `ppvm-traits`' `CorrelatedLossChannel`, and every other site cites it — the two tableau backends, `ppvm-pauli-sum`, `mixins.py`, `paulisum.py` (whose "losing a single qubit" was the ambiguity that let the split through), and the usage skill, which mislabelled the triple as a Pauli-error vector `[p_x, p_y, p_z]` rather than `[p_LL, p_LQ, p_LN]`. Adds `debug_assert`s for the admissible region `p[0], p[1] >= 0`, `p[0] + 2·p[1] <= 1`, `p[2] ∈ [0, 1]` (with 1e-9 slack so a saturated triple like `[1/3, 1/3, _]` is not rejected by rounding). Tests, benches and Python tests that passed out-of-domain triples are corrected to admissible ones without changing a single assertion — e.g. `[0.0, 1.0, 0.0]`, which describes a map that is not completely positive, becomes `[0.0, 0.5, 0.0]`, the same "exactly one lost every shot" witness inside the domain. **2. Duplicate qubit indices silently collapsed in batched Cliffords.** `build_masks` ORs one bit per target, so a repeated index applied the gate once instead of `k` times. `X 0 0` is legal Stim meaning apply-per-target, so `run_string("X 0 0\nM 0")` returned `Some(true)` where the truth is `Some(false)`. Detected now via a popcount against the index count, falling back to the per-index loop, which conjugates by `G^k` correctly for every family — XOR-cancelling the mask would be wrong for `s`/`sqrt_x`/`sqrt_y`, where `S² = Z ≠ I`. The fallback bodies are `#[cold] #[inline(never)]` so the fused sweeps keep their register budget: the ten bit-plane batch rows measure 0.990–1.008× against `origin/main`. **3. The fused CZ block corrupted state when pairs overlapped.** `cz_block` and `cz_block_pairs` assumed disjoint support, which the Lean proves is necessary but nothing enforced. On `X₀X₁X₂`, `cz_block(0, 1, 2)` returned `+Y₀Y₁Y₂` where the per-pair loop gives `−Y₀X₁Y₂` — wrong in both bit planes and in the sign. Since `cz_block(0, 1, n)` is adjacent-pair brickwork, the natural call was the broken one. Now falls back per pair when `offset < count`; disjoint calls stay bit-for-bit on the fused kernel. `cargo test --workspace` green (65 targets), `cargo fmt` clean, and `pytest ppvm-python/test/` 217 passed against a rebuilt wheel. Benchmarked against a pristine `origin/main` checkout across 35 rows including the untouched two-qubit and scalar gates: 0.985–1.021×, all inside the noise floor. Known follow-up, deliberately not in this PR: `ppvm-pauli-sum`'s impl is coefficient-generic, and `Coefficient` carries no ordering, so it documents the admissible region but does not assert it — meaning it still produces negative coefficients where the tableau backends now raise. The guards are also `debug_assert`s, so release wheels gain no protection; surfacing this as a `ValueError` at the binding layer is a separate change. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

Copilot

Pull request overview

This pull request fixes three user-visible correctness defects in the legacy crates that back the shipped ppvm Python wheel: it standardizes the correlated-loss p[1] convention across backends, ensures batched Clifford gates respect duplicate targets, and makes fused CZ-block helpers correct (via fallback) when CZ pair supports overlap.

Changes:

Define and propagate a single normative correlated-loss convention (p[1] is per-named-qubit loss), align tableau trajectory + mixture weighting to 2*p[1], and add admissible-region debug_asserts plus cross-backend regression tests.
Detect repeated indices in batched single-qubit Clifford layers and fall back to per-index loops to apply G^k correctly.
Prevent incorrect fused CZ-block behavior on overlapping pairs by adding preconditions + routing overlapping cases to a per-pair fallback, with new regression tests.

Reviewed changes

Copilot reviewed 12 out of 12 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
skills/ppvm-usage/SKILL.md	Updates usage guidance to label correlated-loss parameters as `[p_LL, p_LQ, p_LN]`.
ppvm-python/test/generalized_tableau/test_loss.py	Fixes tests to use admissible correlated-loss triples and adds a cross-backend agreement test vs `LossyPauliSum`.
ppvm-python/src/ppvm/paulisum.py	Clarifies the correlated-loss docstring, especially the normative `2*p[1]` interpretation.
ppvm-python/src/ppvm/mixins.py	Aligns Python API documentation with the normative `p[1]` convention and documents the admissible region.
crates/ppvm-traits/src/traits/noise.rs	Establishes the normative correlated-loss `p[1]` definition in one place for all backends/bindings to reference.
crates/ppvm-tableau/src/noise.rs	Updates trajectory sampling logic to weight the exactly-one-loss event as `2*p[1]`, adds admissible-region checking, and strengthens tests.
crates/ppvm-tableau/src/gates/clifford.rs	Adds repeated-target detection in mask building and an outlined per-index fallback path for batched Clifford gates, plus regression tests.
crates/ppvm-tableau/src/data.rs	Enforces/records CZ-block disjointness preconditions for the fused kernel and falls back to per-pair CZ when overlaps are possible.
crates/ppvm-tableau/benches/micro.rs	Adjusts correlated-loss benchmark parameters to stay inside the admissible region under the clarified convention.
crates/ppvm-tableau-sum/tests/sampler_vs_pure.rs	Updates mixture-vs-trajectory tests to use the normative `p[1]` convention and admissible triples.
crates/ppvm-tableau-sum/src/noise.rs	Fixes mixture branch weights for correlated loss to use `p[1]` per single-loss branch and survivor weight `1 - p[0] - 2*p[1]`, with new tests.
crates/ppvm-pauli-sum/src/sum/noise.rs	Updates documentation to explicitly reference the trait’s normative correlated-loss convention and document the admissible region behavior.

Suppressed comments (1)

crates/ppvm-tableau/src/gates/clifford.rs:1714

This comment repeats the same incorrect statement that the listed gates are “identity” under repeated application; S², (√X)², and (√Y)² are non-identity Paulis. Adjust wording so the rationale for falling back on repeated targets remains technically correct.

 // *distinct* site. `Gᵏ` is the identity only for the involutory gates
// (`S² = Z`, `(√X)² = X`, `(√Y)² = Y`), so neither one bit nor an
// XOR-cancelled bit is right for every family.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

+/// `Gᵏ`. `Gᵏ` is the identity only for the involutory gates (`S² = Z`,
+/// `(√X)² = X`, `(√Y)² = Y`), so neither one bit nor an XOR-cancelled bit is
+/// right for every family; only the per-index loop is (G-061).


github-actions · 2026-08-12T00:50:20Z

PR Preview Action v1.8.1
Preview removed because the pull request was closed.
2026-09-02 15:15 UTC

david-pl

Some minor comments, but good fixes overall!

david-pl · 2026-09-01T08:16:42Z

+ let (Some(p0), Some(p1), Some(p2)) = (p[0].to_f64(), p[1].to_f64(), p[2].to_f64()) else {
+ return true;
+ };


What is this branch here? If we can't obtain f64, we could still compare to T::Coeff::one and T::Coeff::zero, which is guaranteed by the loss channel trait bounds.

david-pl · 2026-09-01T08:36:18Z

+ debug_assert!(
+ offset >= count,
+ "cz_block pairs must have pairwise-disjoint supports \
+ (Batch.lean::czSeq_phase_needs_disjoint)"
+ );


Why don't we use the same offset >= count check here? In a release build, you can still corrupt things with the Tableau method.

david-pl · 2026-09-01T08:40:39Z

 ```

-Loss channels live on `LossyPauliSum` (same API, plus `loss_channel(q, p)` and `correlated_loss_channel(q0, q1, [p_x, p_y, p_z])`).
+Loss channels live on `LossyPauliSum` (same API, plus `loss_channel(q, p)` and `correlated_loss_channel(q0, q1, [p_LL, p_LQ, p_LN])`).


Maybe clarify the meaning of the probabilities here to avoid issues in the future.

Copilot

🟢 Approval recommended

The changes consistently fix the described correctness issues across all affected backends, add targeted regression tests, and introduce only guarded fallbacks for edge cases without altering established fast paths.

Review details

Files reviewed: 12/12 changed files
Comments generated: 0 new
Review effort level: Lite

Copilot

🟢 Approval recommended

The functional changes are well-scoped, consistent across backends/bindings, and accompanied by targeted regression tests; remaining feedback is limited to minor comment/message clarity.

Review details

Suppressed comments (3)

Previously missed (1) — in code that hasn't changed since the last review.

crates/ppvm-tableau/src/data.rs:242

The debug-assert message says "cz_block pairs" but the function is cz_block_pairs. Using the exact function name in the panic text makes debugging clearer (and avoids looking like two separate APIs).

crates/ppvm-tableau/src/gates/clifford.rs:373

The comment describes S, √X, and √Y as “involutory”, but the examples given have squares S² = Z, (√X)² = X, (√Y)² = Y (i.e., not the identity). This is confusing in a section explaining why a repeated index must apply G^k rather than collapsing bits in a mask.

/// sweep apply the gate once where the per-index loop — the `CliffordBatch`
/// contract, and what a legal `X 0 0` in a `.stim` file means — conjugates by
/// `Gᵏ`. `Gᵏ` is the identity only for the involutory gates (`S² = Z`,
/// `(√X)² = X`, `(√Y)² = Y`), so neither one bit nor an XOR-cancelled bit is
/// right for every family; only the per-index loop is (G-061).

crates/ppvm-tableau/src/gates/clifford.rs:1714

This test-module comment also calls S, √X, and √Y “involutory” while citing S² = Z, (√X)² = X, (√Y)² = Y (not the identity). Clarifying this helps keep the rationale for the duplicate-index fallback precise.

 // apply-to-each-target-in-order, and the fused mask carries one bit per
// *distinct* site. `Gᵏ` is the identity only for the involutory gates
// (`S² = Z`, `(√X)² = X`, `(√Y)² = Y`), so neither one bit nor an
// XOR-cancelled bit is right for every family.

Files reviewed: 12/12 changed files
Comments generated: 0 new
Review effort level: Lite

Copilot

🟢 Approval recommended

The fixes are coherent across Rust/Python surfaces with strong regression coverage, and the only noted issue is a minor comment wording nit with an inline suggestion.

Review details

Files reviewed: 13/13 changed files
Comments generated: 1
Review effort level: Lite

Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>

Copilot

🔵 Needs a closer look

It changes low-level simulation kernels and probabilistic channel semantics across multiple backends, so a final human review is warranted despite the added regression coverage.

Review details

Files reviewed: 13/13 changed files
Comments generated: 0 new
Review effort level: Lite

CopilotAI lite review requested due to automatic review settings August 12, 2026 00:43

Copilotstarted reviewing on behalf of Roger-luo August 12, 2026 00:43 View session

This was referenced Aug 12, 2026
GeneralizedTableauSum(sum_cutoff=0.0) panics on sampler() for any multi-branch channel #206
Open
Noise-channel probability validation is developer-only: debug_assert, PanicException, and no guard at all on PauliSum #207
Open

CopilotAI reviewed Aug 12, 2026
View reviewed changes

david-pl requested changes Sep 1, 2026
View reviewed changes

Merge branch 'main' into fix/legacy-correlated-loss-and-batch-duplicates
b6cdfa5

CopilotAI review requested due to automatic review settings September 2, 2026 14:19

Copilotstarted reviewing on behalf of david-pl September 2, 2026 14:19 View session

CopilotAI reviewed Sep 2, 2026
View reviewed changes

Update crates/ppvm-tableau/src/data.rs
eae704d

CopilotAI review requested due to automatic review settings September 2, 2026 14:32

Copilotstarted reviewing on behalf of david-pl September 2, 2026 14:33 View session

CopilotAI reviewed Sep 2, 2026
View reviewed changes

Some small fixes
2b87a0e

CopilotAI review requested due to automatic review settings September 2, 2026 14:59

david-pl approved these changes Sep 2, 2026
View reviewed changes

Copilotstarted reviewing on behalf of david-pl September 2, 2026 14:59 View session

CopilotAI reviewed Sep 2, 2026
View reviewed changes

Comment threadcrates/ppvm-tableau/src/noise.rs Outdated

Potential fix for pull request finding
db5b09e
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>

CopilotAI review requested due to automatic review settings September 2, 2026 15:07

david-pl enabled auto-merge (squash) September 2, 2026 15:07

Copilotstarted reviewing on behalf of david-pl September 2, 2026 15:08 View session

CopilotAI reviewed Sep 2, 2026
View reviewed changes

david-pl merged commit 70d269b into mainSep 2, 2026
14 checks passed

david-pl deleted the fix/legacy-correlated-loss-and-batch-duplicates branch September 2, 2026 15:14

Navigation Menu

Uh oh!

fix(tableau): correlated-loss convention, duplicate batch targets, CZ-block overlap - #205

fix(tableau): correlated-loss convention, duplicate batch targets, CZ-block overlap#205
david-pl merged 5 commits into
mainfrom
fix/legacy-correlated-loss-and-batch-duplicates

Roger-luo commented Aug 12, 2026

Uh oh!

CopilotAI left a comment

Uh oh!

github-actionsBot commented Aug 12, 2026•
edited
Loading

Uh oh!

david-pl left a comment

Uh oh!

david-plSep 1, 2026

Uh oh!

david-plSep 1, 2026

Uh oh!

Uh oh!

david-plSep 1, 2026

Uh oh!

CopilotAI left a comment

Uh oh!

CopilotAI left a comment

Uh oh!

CopilotAI left a comment

Uh oh!

Uh oh!

CopilotAI left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

Roger-luo commented Aug 12, 2026

1. Correlated loss disagreed between backends by a factor of two

2. Duplicate qubit indices silently collapsed in batched Cliffords

3. The fused CZ block corrupted state when pairs overlapped

Verification

Known follow-ups, deliberately not in this PR

Uh oh!

CopilotAI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

github-actionsBot commented Aug 12, 2026• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

david-pl left a comment

Choose a reason for hiding this comment

Uh oh!

david-plSep 1, 2026

Choose a reason for hiding this comment

Uh oh!

david-plSep 1, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

david-plSep 1, 2026

Choose a reason for hiding this comment

Uh oh!

CopilotAI left a comment

Choose a reason for hiding this comment

🟢 Approval recommended

Uh oh!

CopilotAI left a comment

Choose a reason for hiding this comment

🟢 Approval recommended

Suppressed comments (3)

Uh oh!

CopilotAI left a comment

Choose a reason for hiding this comment

🟢 Approval recommended

Uh oh!

Uh oh!

CopilotAI left a comment

Choose a reason for hiding this comment

🔵 Needs a closer look

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

github-actionsBot commented Aug 12, 2026•
edited
Loading