refactor: shrink dead internal trait surface; document the backend-implementor interface - #188
Merged
Merged
Conversation
Shrink the internal API a simulation-backend implementor must read past. No behavior change; the workspace test suite is unaffected. - Remove the dead `ACMapInsert::map_insert` trait method. - Remove the empty `ptm` module. - Remove the unused `PauliErrorAll` trait. - Drop commented-out code from the hashmap/dashmap map backends. - Add a map-backend implementor's guide documenting the `ACMap` contract. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…eau GPU surface Shrink the trait a word type must implement and document where the tableau crate is already GPU-offloadable. No behavior change; the workspace test suite is unaffected. - Remove four unused `PauliWordTrait` methods (`get_multiple`, `set_multiple`, `get_slice`, `set_new_2`) and their impls. - Document the tableau crate's data-parallel, GPU-offloadable surface: raw-word planes, the shared Clifford loop, `CliffordBatch`/`cz_block`, and `branch_with_coefficients`. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR reduces the internal “backend implementor” trait surface across ppvm-traits / ppvm-pauli-word, and adds documentation describing the intended seams for map backends and data-parallel/GPU-friendly tableau primitives.
Changes:
- Removed unused trait surface:
ACMapInsert::map_insert,PauliErrorAll, and severalPauliWordTraithelper methods, plus related commented-out backend code. - Added an
ACMapbackend implementor’s guide and expanded crate/module docs inppvm-tableaudescribing bulk, data-parallel primitives.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| crates/ppvm-traits/src/traits/word_trait.rs | Removes unused PauliWordTrait helper methods, shrinking the trait surface. |
| crates/ppvm-traits/src/traits/noise.rs | Removes the unused PauliErrorAll trait. |
| crates/ppvm-traits/src/traits/mod.rs | Stops compiling/exporting dead ptm surface and removes the PauliErrorAll re-export. |
| crates/ppvm-traits/src/traits/map.rs | Removes dead map_insert variant and adds map-backend implementor documentation. |
| crates/ppvm-traits/src/map/hashmap.rs | Deletes commented-out/unused map backend code tied to removed trait surface. |
| crates/ppvm-traits/src/map/dashmap.rs | Deletes commented-out/unused map backend code tied to removed trait surface. |
| crates/ppvm-tableau/src/lib.rs | Adds crate-level docs describing data-parallel/GPU-offloadable surface areas. |
| crates/ppvm-tableau/src/data.rs | Adds method-level docs pointing back to the crate’s data-parallel/GPU surface discussion. |
| crates/ppvm-pauli-word/src/word/data.rs | Removes unused PauliWordTrait method impls. |
| crates/ppvm-pauli-word/src/loss/data.rs | Removes unused PauliWordTrait method impls for lossy words. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+152
to
+154
| /// A new backing map (a GPU-resident map, a sharded map, anything beyond | ||
| /// the existing `HashMap` / `IndexMap` / `AHashMap` / `DashMap` backends) | ||
| /// must implement exactly these traits — nothing more: |
Comment on lines
62
to
80
| @@ -69,21 +70,17 @@ pub trait ACMapInsert< | |||
| > | |||
| { | |||
| /// Modify each existing entry in place; if `f` returns `Some((k', v'))`, | |||
| /// also insert that new entry into `dest`. | |||
| fn map_insert<F>(&mut self, dest: &mut Self, f: F) | |||
| where | |||
| F: Fn(&W, &mut V) -> Option<(W, V)> + Sync + Send; | |||
|
|
|||
| /// Like [`map_insert`](Self::map_insert) but appends produced entries | |||
| /// into a plain `Vec` (a push per entry, no hashing) instead of a map. | |||
| /// The caller merges the buffer into the destination map afterwards, | |||
| /// avoiding a second hashmap probe per produced entry. | |||
| /// append that new entry into `dest`, a plain `Vec` (a push per entry, | |||
| /// no hashing). This is the primary hot-path entry point: the caller | |||
| /// merges the buffer into the destination map afterwards, avoiding a | |||
| /// second hashmap probe per produced entry. | |||
| fn map_insert_vec<F>(&mut self, dest: &mut Vec<(W, V)>, f: F) | |||
| where | |||
| F: Fn(&W, &mut V) -> Option<(W, V)> + Sync + Send; | |||
|
|
|||
Comment on lines
64
to
68
| /// [`Pauli`] symbol at `index`. | ||
| fn get(&self, index: usize) -> Pauli; | ||
|
|
||
| /// Build a new word containing only the slots at `indices`. | ||
| fn get_multiple<const Q: usize>(&self, indices: [usize; Q]) -> Self { | ||
| let mut result = Self::new(Q); | ||
| for (i, &idx) in indices.iter().enumerate() { | ||
| result.set(i, self.get(idx)); | ||
| } | ||
| result | ||
| } | ||
|
|
||
| /// Overwrite the slots at `indices` with the slots from `values`. | ||
| fn set_multiple<const Q: usize, B: PauliStorage>(&mut self, indices: [usize; Q], values: &Self); | ||
|
|
||
| /// Build a new word from the contiguous slice `slice` of this one. | ||
| fn get_slice(&self, slice: std::ops::Range<usize>) -> Self; | ||
|
|
||
| /// Quick check: is the slot at `index` equal to `pauli`? | ||
| fn is(&self, index: usize, pauli: Pauli) -> bool; |
Comment on lines
59
to
63
| } | ||
| } | ||
|
|
||
| /// Apply the same single-qubit Pauli error channel uniformly to every | ||
| /// qubit in the system. | ||
| pub trait PauliErrorAll<T: Config> { | ||
| /// Apply `Pauli error` with probabilities `p = [p_x, p_y, p_z]` to | ||
| /// every qubit. | ||
| fn pauli_error_all(&mut self, p: [T::Coeff; 3]); | ||
| } | ||
|
|
||
| /// Two-qubit Pauli error channel. | ||
| pub trait TwoQubitPauliError<T: Config> { |
|
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
It introduces semver-breaking changes to the public ppvm-traits API surface (removed exported traits/methods) without an accompanying versioning/deprecation plan.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (1)
crates/ppvm-traits/src/traits/map.rs:155
- The implementor guide says a new backing map must implement “exactly these traits — nothing more”, but
ACMapalso requires the concrete map type itself to beClone(see thepub trait ACMap: Clone + ...bound just below). As written, this doc is slightly misleading for new backend authors.
/// A new backing map (a GPU-resident map, a sharded map, anything beyond
/// the existing `HashMap` / `IndexMap` / `AHashMap` / `DashMap` backends)
/// must implement exactly these traits — nothing more:
///
- Files reviewed: 10/10 changed files
- Comments generated: 1
- Review effort level: Lite
Comment on lines
26
to
29
| pub use noise::{ | ||
| AmplitudeDamping, AsymmetricLossChannel, CorrelatedLossChannel, Depolarizing, Depolarizing2, | ||
| LossChannel, PauliError, PauliErrorAll, ResetLossChannel, TwoQubitPauliError, | ||
| LossChannel, PauliError, ResetLossChannel, TwoQubitPauliError, | ||
| }; |
…haned ptm.rs Review follow-ups on the internal-interface shrink: - `mod ptm;` was removed but `traits/ptm.rs` was left in the tree as an orphaned source file the compiler never sees. Delete it. - The tableau crate-level doc claimed every Clifford caller funnels through one shared per-gate implementation. The `CliffordBatch` path does not: `x_many`/`cz_many` carry their own raw-plane loops, and `Tableau` and `GeneralizedTableau` have separate impls. Say so. - The same doc described `CliffordBatch` as operating on a contiguous block of qubits; it takes arbitrary index lists. Only `cz_block` and friends are block-contiguous. - The `ACMap` implementor's guide claimed its list was exhaustive. It omitted the `Clone` supertrait and listed `ACMapIter`, which is not an `ACMap` bound. Split the list into the `ACMap` bound proper and the traits `ppvm-pauli-sum` additionally requires (`ACMapIter`, `Trace`, `Extend`, `PartialEq`). - The same guide claimed `DashMap` implements every batch method with rayon. `retain` is sequential — its `FnMut` predicate is not shareable across threads. Name the exception. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
🟢 Approval recommended
The changes are confined to dead-code removal and documentation updates, and repository-wide searches show no remaining references to the removed APIs/modules.
Review details
- Files reviewed: 11/11 changed files
- Comments generated: 0 new
- Review effort level: Lite
david-pl
enabled auto-merge (squash)
September 2, 2026 14:05
david-pl
disabled auto-merge
September 2, 2026 14:09
david-pl
enabled auto-merge (squash)
September 2, 2026 14:09
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 free
to 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.
Summary
Shrinks the internal developer interface — the trait surface someone must read and implement to add a new simulation backend or a specialized (multi-threaded / GPU) execution strategy — by removing dead surface and documenting the seams that already exist. Net −127 lines (82 insertions, 209 deletions) across
ppvm-traits,ppvm-pauli-word, andppvm-tableau.Two commits, each a behavior-neutral cleanup:
refactor(traits): remove dead map-backend trait surface— drops the deadACMapInsert::map_insertmethod, the emptyptmmodule, the unusedPauliErrorAlltrait, and commented-out code in the hashmap/dashmap backends; adds a map-backend implementor's guide for theACMapcontract.refactor(pauli-word): drop dead PauliWordTrait methods; document tableau GPU surface— removes four unusedPauliWordTraitmethods (get_multiple,set_multiple,get_slice,set_new_2) and their impls; documents the tableau crate's data-parallel, GPU-offloadable surface (raw-word planes, the shared Clifford loop,CliffordBatch/cz_block,branch_with_coefficients).Correctness
cargo test --workspacepasses;cargo check --workspaceis clean;cargo fmt/clippyand the license-header hooks pass (this branch is the integration of iterations that each cleared those gates).Provenance
Produced by an autonomous, rubric-driven autotune run whose objective was three LLM-judge rubrics — developer-interface simplicity, multi-threading friendliness, and CUDA-kernel friendliness — scored against the actual code, with the criterion benchmarks (
tableau-msd,pauli-sumtrotter) held as guardrails so no change could regress performance. These are the two iterations the loop kept (guardrails reproduced, no regression) before the rubric scores plateaued. Rubric trajectory across the kept changes: simplicity 5→8, multi-threading 7→8, CUDA-friendliness 3→5.🤖 Generated with Claude Code