Uh oh!
There was an error while loading. Please reload this page.
Draft implementation of generalized tableau simulator - #34
Merged
Conversation
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
david-pl
marked this pull request as ready for review
April 3, 2026 15:15
| /// * Entries 0..n are the destabilizers | ||
| /// * Entries n..2n are the stabilizers | ||
| pub data: Vec<PhasedPauliWordNoHash<T::Storage, T::BuildHasher>>, | ||
| pub(crate) rng: SmallRng, |
Collaborator
There was a problem hiding this comment.
would it be problematic if the RNG is not global?
CollaboratorAuthor
There was a problem hiding this comment.
You mean not pub(crate)? I suppose we could use accessor functions instead, but not sure what the benefit would be. We do need to access it here and there to generate a sample.
| use rand::SeedableRng; | ||
| use rand::rngs::SmallRng; | ||
| type PhasedPauliWordNoHash<A, H> = PhasedPauliWord<A, H, PauliWord<A, H, false>>; |
Collaborator
There was a problem hiding this comment.
why do we need a NoHash? just curious
CollaboratorAuthor
There was a problem hiding this comment.
It's a performance improvement. Otherwise we .rehash after every Clifford gate, which sums up to significant performance cost and is useless for tableau simulation.
Uh oh!
There was an error while loading. Please reload this page.
Comment on lines
+471
to
+474
| let cutoff = Complex { | ||
| re: self.coefficient_threshold.clone(), | ||
| im: T::Coeff::zero(), | ||
| }; |
Collaborator
There was a problem hiding this comment.
it seems one can just use the coefficient_threshold directly to compare with the coeff.abs() what's the reason of creating a complex number first then calculate abs here?
CollaboratorAuthor
There was a problem hiding this comment.
Yes, that was dumb. I fixed that though. I still compute .abs due to trait bounds.
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>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
FYI, @rafaelha@Roger-luo this is a draft of a simulator similar to SOFT, but CPU only.
Current status:
u128as index)PhasedPauliWord. Note, that this also means we have row-major data storage.There's an example for the MSD circuit and the 85 qubit version runs in ~1.5ms on my laptop for a single shot.
Open TODOs:
Better sparse vector (currently that's just aNeed a custom impl since we need arbitrary bit width index support.Vec<(Complex64, u128)>PhasedPauliWordsince it carries aroundConfigwithout really being needed. Or we can refactor it? For now though, it's fine, I think.Also, a bit of an open research question is how we could do weight truncation here. Maybe we can identify indices in the
coefficientsthat correspond to those and truncate them. But it's not so clear.