Repository navigation
refactor(traits): consolidate noise/Clifford impls and trim where clauses - #57
Conversation
|
There was a problem hiding this comment.
Pull request overview
This PR refactors and consolidates stabilizer noise-channel and Clifford-gate implementations across ppvm-tableau and ppvm-runtime, reducing duplication and simplifying generic bounds while preserving existing test coverage.
Changes:
- Introduces a new
TableauLiketrait inppvm-tableauproviding default implementations for common single-/two-qubit Pauli noise channels. - Adds a blanket
Clifford/CliffordExtensionsimplementation for allPauliWordTraitimplementors, deleting duplicatedPauliWord/LossyPauliWordgate logic. - Simplifies
PauliSumimpl generics and coefficient arithmetic to reduce where-clause clutter and removef64 op T::Coeffbounds.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| crates/ppvm-tableau/src/tableau_like.rs | Adds TableauLike trait with default noise-channel implementations. |
| crates/ppvm-tableau/src/noise.rs | Delegates tableau noise traits to TableauLike default methods. |
| crates/ppvm-tableau/src/lib.rs | Exposes tableau_like module and re-exports TableauLike in prelude. |
| crates/ppvm-runtime/src/traits/clifford.rs | Adds blanket Clifford + CliffordExtensions impls for PauliWordTrait. |
| crates/ppvm-runtime/src/traits/word_trait.rs | Removes Clifford supertrait requirement; documents blanket gate behavior. |
| crates/ppvm-runtime/src/word/clifford.rs | Removes concrete PauliWord Clifford impl; retains tests. |
| crates/ppvm-runtime/src/loss/clifford.rs | Removes concrete LossyPauliWord Clifford impl; retains tests. |
| crates/ppvm-runtime/src/sum/noise.rs | Rewrites coefficient arithmetic and consolidates repeated expressions via helper. |
| crates/ppvm-runtime/src/sum/clifford.rs | Simplifies PauliSum Clifford impl bounds to impl<T: Config>. |
| crates/ppvm-runtime/src/sum/rot1.rs | Collapses Config<Storage=..., BuildHasher=...> bounds to T: Config. |
| crates/ppvm-runtime/src/sum/rot2.rs | Collapses Config<Storage=..., BuildHasher=...> bounds to T: Config. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| pub trait TableauLike: Clifford { | ||
| /// Coefficient type used for probabilities. | ||
| type Coeff: Coefficient + PartialOrd<f64> + Zero; | ||
|
|
There was a problem hiding this comment.
TableauLike::Coeff is bounded by Coefficient + ... + Zero, but Coefficient already requires num::Zero. Keeping both is redundant and can be removed (and use num::Zero; may no longer be necessary) to simplify the public trait signature.
| pub trait TableauLike: Clifford { | ||
| /// Coefficient type used for probabilities. | ||
| type Coeff: Coefficient + PartialOrd<f64> + Zero; | ||
|
|
||
| /// Mutable access to the backend's RNG. | ||
| fn rng_mut(&mut self) -> &mut SmallRng; | ||
|
|
There was a problem hiding this comment.
TableauLike hard-codes SmallRng in the public API (rng_mut returns &mut SmallRng). If this trait is intended for extensible backends, consider using an associated RNG type (e.g., type Rng: rand::Rng) or &mut impl rand::RngCore so implementors aren’t forced to store SmallRng specifically.
| /// Single-qubit Pauli-error channel (X, Y, Z with given probabilities). | ||
| #[inline] | ||
| fn pauli_error_impl(&mut self, addr0: usize, p: [Self::Coeff; 3]) { | ||
| if self.is_qubit_lost(addr0) { | ||
| return; | ||
| } | ||
| let r = self.rng_mut().random::<f64>(); | ||
| let mut cumulative = Self::Coeff::zero(); | ||
| for (i, p_) in p.iter().enumerate() { | ||
| cumulative += p_.clone(); | ||
| if cumulative > r { | ||
| match i { | ||
| 0 => self.x(addr0), | ||
| 1 => self.y(addr0), | ||
| _ => self.z(addr0), | ||
| } | ||
| return; | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
The new default pauli_error_impl no longer performs the debug-time probability validation that previously existed in the specialized implementation (e.g., ensuring each entry is in [0,1] and the total is near 1). Consider restoring equivalent debug_assert! checks here so callers still get early feedback in debug builds.
| /// Two-qubit Pauli-error channel (15 non-identity Pauli combinations). | ||
| #[inline] | ||
| fn two_qubit_pauli_error_impl(&mut self, addr0: usize, addr1: usize, p: [Self::Coeff; 15]) { | ||
| if self.is_qubit_lost(addr0) || self.is_qubit_lost(addr1) { | ||
| return; | ||
| } | ||
| let r = self.rng_mut().random::<f64>(); | ||
| let sum = Self::Coeff::zero(); | ||
| let idx = p | ||
| .iter() | ||
| .scan(sum, |acc, p_| { | ||
| *acc += p_.clone(); | ||
| Some(acc.clone()) | ||
| }) | ||
| .position(|cum_prob| cum_prob > r); | ||
|
|
There was a problem hiding this comment.
The new default two_qubit_pauli_error_impl drops the previous debug-time validation of the 15 probabilities (range checks / sum sanity). Consider reintroducing those debug_assert! checks in this shared implementation to preserve the prior developer feedback and make misuse easier to catch.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 11 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| impl<T: PauliWordTrait> Clifford for T { | ||
| #[inline] | ||
| fn x(&mut self, _index: usize) { | ||
| // X * I * X = I 00 -> 00, 0 |
There was a problem hiding this comment.
The blanket impl<T: PauliWordTrait> Clifford for T is a breaking/coherence-affecting change for downstream crates: any external PauliWordTrait type that already has its own impl Clifford will now conflict, and downstreams can no longer provide a custom Clifford implementation for a PauliWordTrait type. Consider documenting this explicitly as an API-breaking change (or moving the blanket impl behind an opt-in marker/sealed trait to allow customization).
| //! Any type implementing [`TableauLike`] gets default implementations of | ||
| //! single- and two-qubit Pauli noise channels (Depolarizing, PauliError, | ||
| //! Depolarizing2, TwoQubitPauliError). New tableau backends only need to | ||
| //! provide [`TableauLike::rng_mut`] and (optionally) override | ||
| //! [`TableauLike::is_qubit_lost`]. |
There was a problem hiding this comment.
The module-level docs say implementors only need to provide rng_mut, but the trait actually requires returning &mut SmallRng specifically. To avoid misleading new backend implementors, consider updating the docs to call out the SmallRng requirement (or generalize the trait to accept any RngCore if that's intended).
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 11 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| /// specialized `Clifford` impl instead, the way `PhasedPauliWord` does. The | ||
| /// blanket impl will otherwise shadow your override. |
There was a problem hiding this comment.
Doc note: implementing PauliWordTrait and also providing a custom impl Clifford for ... is not something that can be “shadowed” by the blanket impl; it will be a conflicting/overlapping impl error. Consider rewording this sentence to explicitly say it won’t compile, and that custom Clifford semantics require not implementing PauliWordTrait for that type.
| /// specialized `Clifford` impl instead, the way `PhasedPauliWord` does. The | |
| /// blanket impl will otherwise shadow your override. | |
| /// specialized `Clifford` impl instead, the way `PhasedPauliWord` does. | |
| /// Implementing both `PauliWordTrait` and a custom `impl Clifford for ...` | |
| /// for the same type will not compile because the blanket impl would | |
| /// conflict/overlap with your custom impl. |
| fn pauli_error_impl(&mut self, addr0: usize, p: [Self::Coeff; 3]) { | ||
| let r = self.rng_mut().random::<f64>(); | ||
| let mut cumulative = Self::Coeff::zero(); | ||
| for (i, p_) in p.iter().enumerate() { | ||
| cumulative += p_.clone(); | ||
| if cumulative > r { |
There was a problem hiding this comment.
pauli_error_impl no longer has the debug-time validation that previously existed for GeneralizedTableau::pauli_error (bounds check and approx-sum check). Consider restoring at least a debug_assert! that each entry is in [0, 1] (and optionally that the total probability does not exceed 1) to catch invalid inputs early in debug builds.
| fn depolarize2_impl(&mut self, addr0: usize, addr1: usize, p: Self::Coeff) { | ||
| if self.is_qubit_lost(addr0) || self.is_qubit_lost(addr1) { | ||
| return; | ||
| } | ||
| let p_arr: [Self::Coeff; 15] = core::array::from_fn(|_| p.clone() * (1.0 / 15.0)); | ||
| self.two_qubit_pauli_error_impl(addr0, addr1, p_arr); |
There was a problem hiding this comment.
depolarize2_impl currently accepts any p and distributes it across 15 errors without even a debug-time [0, 1] check (previously indirect via the two-qubit helper’s asserts). Consider adding a debug_assert!(p >= 0.0 && p <= 1.0) here as well, since this method is a public default implementation.
…re debug_asserts, clarify doc - Drop `+ Zero` from `TableauLike::Coeff` bound (already implied by `Coefficient: num::Zero`). Keep the `use num::Zero;` since default method bodies still call `Self::Coeff::zero()`. - Restore `debug_assert!` probability-range checks on `pauli_error_impl`, `two_qubit_pauli_error_impl`, and `depolarize2_impl`. These were present on the original GT noise impls and got lost when the logic moved into the shared `TableauLike` defaults. (The old `sum ≈ 1.0` assert is intentionally not restored — partial-error distributions do not sum to 1.) - Reword the "shadowing" note on `PauliWordTrait`: a custom `impl Clifford for T` on a `PauliWordTrait` type triggers a coherence error, not silent shadowing. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…hange Address PR #57 review: downstream `impl Clifford for MyWord where MyWord: PauliWordTrait` will now overlap with the blanket impl and fail to compile. Document the constraint and point readers at `PhasedPauliWord` as the pattern for custom Clifford semantics. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…uses - Introduce TableauLike trait in ppvm-tableau with default impls for single-/two-qubit Pauli noise channels (Depolarizing, PauliError, Depolarizing2, TwoQubitPauliError). Uses an associated Rng type so backends aren't forced to use SmallRng. Restores debug_assert! checks for probability ranges and adds early-return on lost qubits. - Add blanket impl<T: PauliWordTrait> Clifford for T (and CliffordExtensions), removing duplicated PauliWord / LossyPauliWord gate logic. Documented as a breaking change for downstream crates: any external impl Clifford for a PauliWordTrait type now conflicts with the blanket impl. - Simplify PauliSum impl generics: collapse Config<Storage=..., BuildHasher=...> bounds to T: Config and drop redundant f64-op-Coeff bounds in noise/clifford/rot1/rot2 modules. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
9ad7d2d to
6f20ed7
Compare
Summary
Four independent trait-system improvements, net -402 lines. All 199 library tests pass; Python bindings still compile.
sum/noise.rsarithmetic: rewrotef64 op T::CoeffasT::Coeff op f64, eliminatingf64: Mul/Add/Sub<T::Coeff>bounds from 7 impl blocks. The 15-armTwoQubitPauliErrormatch now uses a smallone_minus_two_sumhelper instead of 8-line repeated expressions.TableauLiketrait (new, inppvm-tableau): carries default implementations ofdepolarize_impl,pauli_error_impl,two_qubit_pauli_error_impl,depolarize2_impl.TableauandGeneralizedTableauimplement it with justrng_mut(and, for GT,is_qubit_lost); their noise trait impls collapse to one-line delegations. Extensibility: new tableau-like backends inherit all Pauli noise channels for free.Blanket
CliffordforPauliWordTrait: removedCliffordfromPauliWordTrait's supertraits and addedimpl<T: PauliWordTrait> Clifford for Tplus a matchingCliffordExtensionsblanket. Loss guards useget_lbit()— which returnsfalseforPauliWordand the actual bit forLossyPauliWord— so the guard is optimized away on the non-lossy type. This deletes ~380 lines of duplicated gate logic acrossword/clifford.rsandloss/clifford.rs(tests retained). Extensibility: newPauliWordTraittypes automatically get correct Clifford behavior.PauliSumwhere-clause cleanup:impl<S, H, T> ... where T: Config<Storage = S, BuildHasher = H>collapses toimpl<T: Config>usingT::Storage/T::BuildHasherdirectly in theClifford,RotationOne,RotationTwoimpls.Two planned phases were dropped after investigation:
PartialOrd<f64>toCoefficient: blocked becauseComplex<f64>doesn't (and can't) implement ordering.ACMapsub-traits: current granularity is load-bearing —sum/rot1.rs/rot2.rsdepend only onACMapInsert + ACMapConsume, which is the interface-segregation principle at work.Plan file:
~/.claude/plans/generic-hopping-kay.md(local, not committed).Test plan
cargo test --workspace --lib --exclude ppvm-python-nativepasses (199 tests)cargo test --workspace --lib --exclude ppvm-python-native --all-featurespassescargo build -p ppvm-python-nativesucceedscargo clippy --workspace --exclude ppvm-python-nativecleancargo bench -p ppvm-tableau --bench microshows no regression (noise channels are now one extra indirection via trait default method — should inline)🤖 Generated with Claude Code