Repository navigation
Conversation
…uliWord. Forgot to include enum of LossySite in loss.rs in ppvm-traits-2
|
david-pl
left a comment
david-pl
left a comment
There was a problem hiding this comment.
Overall looks good, and thanks for keeping the scope small! There are some copy & paste errors from the standard Pauli word, which don't properly handle caches or lbits.
david-pl
left a comment
david-pl
left a comment
There was a problem hiding this comment.
Some more small findings here.
| fn set_x_bit(&mut self, i: usize, v: bool) { | ||
| debug_assert!(i < self.nqubits, "index {i} out of bounds"); | ||
| self.xbits.set(i, v); | ||
| if v && self.lbits[i] { |
There was a problem hiding this comment.
I'm actually not sure if this is the behavior we want. We implicitly set the loss state here, it's easy to forget that, I think.
And, it requires custom Clifford impls as well as custom rotation impls. Right now, with #253 there's a bug in the latter: rotations will silently reset lost qubits. We can either fix it there or really think about whether we want to have loss semantics down to the level of setters. That is, do we want to skip doing anything here if self.lbits[i] == true? Would have to make the same change throughout, so not sure. Do you happen to know the original motivation behind having it like this?
There was a problem hiding this comment.
I think the motivation was to preserve the invariant that lost sites have zero X/Z bits, but clearing loss in a bit setter also implicitly restores the site.
I’ve changed the behavior so bit setters and toggle builders skip lost sites. Restoring a site now requires an explicit set(..., Present(...)) or clear_loss().
| } | ||
|
|
||
| /// Number of non-identity factors. | ||
| fn weight(&self) -> usize { |
There was a problem hiding this comment.
Counting one bit at a time can be slow. PauliWord::weight already has an efficient version.
| }; | ||
| self.xbits.set(qb, x); | ||
| self.zbits.set(qb, z); | ||
| self.invalidate_xz_hash(); |
There was a problem hiding this comment.
We always invalidate hashes in the set_* methods. I think the original PauliWord avoids this by checking the current bit configuration.
Krastanov
left a comment
Krastanov
left a comment
There was a problem hiding this comment.
some minor notes after AI-assisted review mostly related to debug asserts that might need to be present in release too (I am not sure of the typical rust conventions here, so take it with a grain of salt)
I am new to the codebase so I still heavily rely on conversations with an LLM to do reviews.
|
|
||
| impl<A: PauliStorage, H> Eq for LossyPauliWord<A, H> {} | ||
|
|
||
| impl<A: PauliStorage, H> From<&str> for LossyPauliWord<A, H> { |
There was a problem hiding this comment.
LossyPauliWord::<u8>::from("IIIIIIIII") succeeds in both debug and release, reports nine sites, then panics when formatted or iterated. "XXXXXXXXI" has the same problem: identity symbols perform no bit write, so they bypass storage bounds checks, and from_planes never checks capacity.
| Self::from_planes(xbits, zbits, lbits, self.nqubits) | ||
| } | ||
|
|
||
| pub fn set(&mut self, qb: usize, site: LossySite<Pauli>) { |
There was a problem hiding this comment.
Should this be just debug_assert!? Under --release, constructing "I" and calling set(1, Present(X)) succeeds because index 1 fits the backing storage. The word still displays "I", but its weight becomes 1 and it compares unequal to a freshly parsed "I".
| } | ||
|
|
||
| /// Number of non-identity factors. | ||
| fn weight(&self) -> usize { |
There was a problem hiding this comment.
David already noticed this, here are some microbenchmarks that codex did for me
Operation / storage Legacy PR 250 Chunk-iterator alternative
━━━━━━━━━━━━━━━━━━━━━━━ ━━━━━━━━━ ━━━━━━━━━━ ━━━━━━━━━━━━━━━━━━━━━━━━━━━━
weight, u64 0.76 ns 2.29 ns 0.84 ns
─────────────────────── ───────── ────────── ────────────────────────────
weight, [u64; 4] 1.66 ns 16.59 ns 1.75 ns
─────────────────────── ───────── ────────── ────────────────────────────
loss_weight, u64 0.73 ns 2.91 ns 0.74 ns
─────────────────────── ───────── ────────── ────────────────────────────
loss_weight, [u8; 32] 1.44 ns 16.83 ns 1.44 ns
…are not done implicitly
This branch has not been deployed
3 participants
As this crate can be quite complicated, this is just PR 1 of incremental merges to support lossy-pauli-word-2. This patch does the following:
Part of the effort to refactoring #204