refactor: shrink dead internal trait surface; document the backend-implementor interface#188
Open
Roger-luo wants to merge 2 commits into
Open
refactor: shrink dead internal trait surface; document the backend-implementor interface#188Roger-luo wants to merge 2 commits into
Roger-luo wants to merge 2 commits into
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> { |
|
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