Skip to content

Implement correlated loss - #38

Merged
david-pl merged 8 commits into
mainfrom
david/30-correlated-loss
Mar 16, 2026
Merged

Implement correlated loss#38
david-pl merged 8 commits into
mainfrom
david/30-correlated-loss

Conversation

@david-pl

Copy link
Copy Markdown
Collaborator

No description provided.

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This pull request adds support for a new correlated loss noise channel across the Rust runtime and the Python bindings, with tests validating expected behavior on small examples (including a GHZ-style circuit).

Changes:

  • Introduces CorrelatedLossChannel and implements correlated-loss behavior in ppvm-runtime.
  • Exposes correlated_loss_channel through ppvm-python-native and adds a Python wrapper + tests.
  • Updates Python tooling metadata (lockfile registry/version changes; removes a pre-commit hook).

Reviewed changes

Copilot reviewed 12 out of 13 changed files in this pull request and generated 5 comments.

Show a summary per file
FileDescription
ppvm-python/uv.lockLarge lockfile rewrite: registry switch to PyPI + multiple dependency version bumps.
ppvm-python/test/test_loss.pyAdds Python tests for correlated loss on ZZ/XX under a GHZ-like circuit.
ppvm-python/src/ppvm/paulisum.pyAdds LossyPauliSum.correlated_loss_channel() wrapper and docstring.
ppvm-python/.pre-commit-config.yamlRemoves the typos hook from pre-commit.
crates/ppvm-runtime/tests/loss.rsAdds Rust tests for correlated loss channel behavior and GHZ expectation checks.
crates/ppvm-runtime/src/traits/noise.rsAdds CorrelatedLossChannel trait; updates docs for two-qubit Pauli error ordering.
crates/ppvm-runtime/src/traits/mod.rsRe-exports the new CorrelatedLossChannel trait.
crates/ppvm-runtime/src/traits/map.rsExtends ACMapInsert with map_insert_multiple.
crates/ppvm-runtime/src/sum/noise.rsImplements CorrelatedLossChannel for PauliSum using map_insert_multiple.
crates/ppvm-runtime/src/sum/data.rsAdds PauliSum::map_insert_multiple helper method.
crates/ppvm-runtime/src/map/hashmap.rsImplements map_insert_multiple for the HashMap-backed map type.
crates/ppvm-runtime/src/map/dashmap.rsImplements map_insert_multiple for the DashMap-backed map type (parallel).
crates/ppvm-python-native/src/interface.rsExposes correlated loss method in the Python native interface (and truncates after applying).
Comments suppressed due to low confidence (1)

ppvm-python/.pre-commit-config.yaml:26

  • The typos pre-commit hook was removed. If this was not intentional for the correlated-loss feature work, consider keeping it (or documenting why it needs to be removed), since it helps catch spelling issues in code/comments (and this PR introduces at least one typo elsewhere).
- repo: https://github.com/charliermarsh/ruff-pre-commit
# Ruff version.
rev: "v0.13.2"
hooks:
- id: ruff

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment threadcrates/ppvm-runtime/src/traits/noise.rs Outdated
Comment threadppvm-python/src/ppvm/paulisum.py Outdated
Comment threadppvm-python/src/ppvm/paulisum.py Outdated
Comment threadcrates/ppvm-runtime/src/map/hashmap.rs Outdated
Comment threadcrates/ppvm-runtime/src/map/dashmap.rs
david-pland others added 4 commits March 3, 2026 11:33
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 12 out of 13 changed files in this pull request and generated 3 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment threadcrates/ppvm-runtime/src/traits/noise.rs
Comment threadppvm-python/src/ppvm/paulisum.py
Comment threadppvm-python/.pre-commit-config.yaml
@david-pl
david-pl requested a review from Roger-luoMarch 3, 2026 11:39
@david-pl
david-pl merged commit cfbfba0 into mainMar 16, 2026
4 checks passed
david-pl added a commit that referenced this pull request Sep 2, 2026
…-block overlap (#205)
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_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, _]` 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:
```rust
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_assert`s, 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](https://claude.com/claude-code)
---------
Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
Co-authored-by: David Plankensteiner <david-pl@users.noreply.github.com>
Co-authored-by: David Plankensteiner <dplankensteiner@quera.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@david-pl@Roger-luo