Skip to content

Commit ca8fa57

Browse files
Roger-luoclaude
andcommitted
revert(runtime): remove the unsound Strategy::discard insertion-pruning
Reverts the iteration-4 `map_insert` optimization (the `Strategy::discard` predicate and the prune-on-insert branch). As the Copilot review on #117 noted, it changes results whenever `truncate()` is *deferred* across gates: it skips inserting a produced term whose coefficient is individually below the threshold and whose key is absent, on the assumption the next call is always `truncate()`. When several gates run between truncations, two sub-threshold contributions to the same key can sum above the threshold (`|a + b| ≥ τ` while `|a|, |b| < τ`), so dropping either at production loses a genuinely surviving term and biases the terms it would have fed — the same "never discard mass that summed is ≥ τ" violation the earlier iteration-5 revert (1df64df) cared about, just spread across gates instead of one merge. `map_insert` returns to unconditionally merging produced terms via `add_assign` (keeping the iteration-3 reusable scratch `Vec`); truncation is once again the only place terms are dropped. The correct micro-optimizations on this branch — the `rzz`/`rxx`/`ryy` fast paths and the scratch buffer — are untouched. Removes: the `discard` trait method and its `CoefficientThreshold` / `CombinedStrategy` overrides, plus the now-moot "deliberately no override" notes on `MaxPauliWeight` / `MaxLossWeight`. Adds `tests/truncation_semantics.rs`: a deferred-truncation guard (two `rx` on a single `Z`, no truncate between) asserting the `CoefficientThreshold` result equals a non-truncating reference filtered once at the end. The merged `Y` term (≈0.060 > τ=0.05) must survive — it would have been dropped by the removed pruning. This is the regression test the optimization lacked. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
1 parent 18f97c1 commit ca8fa57

5 files changed

Lines changed: 88 additions & 77 deletions

File tree

‎crates/ppvm-runtime/src/strategy.rs‎

Lines changed: 0 additions & 32 deletions
Original file line numberDiff line numberDiff line change
@@ -27,15 +27,6 @@ impl<S1: Strategy, S2: Strategy> Strategy for CombinedStrategy<S1, S2> {
2727
self.0.truncate(map);
2828
self.1.truncate(map);
2929
}
30-
31-
#[inline]
32-
fn discard<V, W>(&self, key: &W, value: &V) -> bool
33-
where
34-
V: Coefficient,
35-
W: PauliWordTrait,
36-
{
37-
self.0.discard(key, value) || self.1.discard(key, value)
38-
}
3930
}
4031

4132
/// Drop terms whose Pauli weight (number of non-identity slots) exceeds
@@ -89,13 +80,6 @@ impl Strategy for MaxPauliWeight {
8980
{
9081
map.retain(|k, _| k.weight() <= self.max_weight());
9182
}
92-
93-
// Deliberately no `discard` override: Pauli weight is *not* monotonic
94-
// under gate propagation (a gate can map a high-weight term to a
95-
// lower-weight one), so skipping insertion of an over-weight term could
96-
// suppress a valid lower-weight term it would have produced in a later
97-
// operation. The default (`false`, never discard) keeps `map_insert`
98-
// exact for this strategy.
9983
}
10084

10185
/// Drop terms whose coefficient magnitude falls below the given threshold.
@@ -140,15 +124,6 @@ impl Strategy for CoefficientThreshold {
140124
{
141125
map.retain(|_, v| !v.cutoff(self.0));
142126
}
143-
144-
#[inline]
145-
fn discard<V, W>(&self, _key: &W, value: &V) -> bool
146-
where
147-
V: Coefficient,
148-
W: PauliWordTrait,
149-
{
150-
value.cutoff(self.0)
151-
}
152127
}
153128

154129
/// Drop terms whose loss weight (number of lost qubits) exceeds the
@@ -179,11 +154,4 @@ impl Strategy for MaxLossWeight {
179154
{
180155
map.retain(|k, _| k.loss_weight() <= self.0);
181156
}
182-
183-
// Deliberately no `discard` override: loss weight is *not* monotonic
184-
// under propagation — `loss_channel` rewrites `L → I`, lowering a term's
185-
// loss weight, so an over-weight term can still produce a valid in-budget
186-
// term in a later channel. Skipping its insertion would drop that
187-
// descendant. The default (`false`, never discard) keeps `map_insert`
188-
// exact for this strategy.
189157
}

‎crates/ppvm-runtime/src/sum/data.rs‎

Lines changed: 1 addition & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -214,31 +214,13 @@ impl<T: Config> PauliSum<T> {
214214
// no hashing), then merge them into the primary map in one pass. This
215215
// avoids the extra hashmap probe + insert the old aux-map buffer paid
216216
// for every produced term, and makes the per-call clear O(1).
217-
let strategy = self.strategy; // Strategy: Copy
218-
// Only prune when no preserve-set is active; otherwise a preserved-
219-
// but-droppable term would be wrongly skipped (truncate re-inserts
220-
// such terms, so they must exist for that mechanism to see them).
221-
let prune = self.preserve_strings.is_empty();
222-
223217
let mut scratch = std::mem::take(&mut self.scratch);
224218
scratch.clear();
225219
self.data_mut().map_insert_vec(&mut scratch, f);
226220

227221
let data = self.data_mut();
228222
for (k, v) in scratch.drain(..) {
229-
if prune && strategy.discard(&k, &v) {
230-
// The next `truncate` would drop this term. Keep it only if
231-
// it merges into an existing entry (preserving that entry's
232-
// value); if its key is absent, skip the insert — avoiding an
233-
// insert now and the index rebuild the following truncate
234-
// would trigger. Identical net result to "insert then
235-
// truncate".
236-
if data.contains_with(&k, |_| true) {
237-
data.add_assign(k, v);
238-
}
239-
} else {
240-
data.add_assign(k, v);
241-
}
223+
data.add_assign(k, v);
242224
}
243225
self.scratch = scratch;
244226
}

‎crates/ppvm-runtime/src/traits/strategy.rs‎

Lines changed: 0 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -28,22 +28,6 @@ pub trait Strategy: Default + Clone + Copy {
2828
H: std::hash::BuildHasher + Clone + Default,
2929
W: PauliWordTrait,
3030
M: ACMap<S, V, H, W>;
31-
32-
/// Whether a freshly produced term `(key, value)` would be dropped by
33-
/// this strategy's [`truncate`](Self::truncate). Gate propagation uses
34-
/// this to skip inserting new terms a following `truncate` would remove
35-
/// anyway. The default returns `false` ("never discard") so any strategy
36-
/// that does not override it — including [`NoStrategy`] and user
37-
/// strategies — simply forgoes the optimization and behaves unchanged.
38-
#[inline]
39-
fn discard<V, W>(&self, key: &W, value: &V) -> bool
40-
where
41-
V: Coefficient,
42-
W: PauliWordTrait,
43-
{
44-
let _ = (key, value);
45-
false
46-
}
4731
}
4832

4933
/// Strategy that never truncates — exact simulation, all entries kept.

‎crates/ppvm-runtime/tests/trotter.rs‎

Lines changed: 8 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -12,11 +12,10 @@
1212
//! 1. **Math regression** — the exact (untruncated, `NoStrategy`) expectation
1313
//! value matches a frozen golden constant.
1414
//! 2. **Truncation fidelity** — the `CoefficientThreshold(1e-6)` run stays
15-
//! within a tight tolerance of the exact run. This is the guard that the
16-
//! insertion-pruning fast path in `map_insert` must remain faithful to
17-
//! standard magnitude truncation: an over-aggressive approximation (e.g.
18-
//! dropping sub-threshold contributions to surviving terms) would drift
19-
//! past the bound and fail here.
15+
//! within a tight tolerance of the exact run, confirming magnitude
16+
//! truncation drops only genuinely negligible mass: an over-aggressive
17+
//! approximation (e.g. dropping sub-threshold contributions to surviving
18+
//! terms) would drift past the bound and fail here.
2019
2120
use ppvm_runtime::prelude::*;
2221
use ppvm_runtime::strategy::CoefficientThreshold;
@@ -108,11 +107,10 @@ fn trotter_result_is_stable_and_truncation_is_faithful() {
108107
);
109108

110109
// (2) Truncation-fidelity guard: the truncated run must stay within one
111-
// truncation unit (1e-6) of exact. Faithful magnitude truncation — what
112-
// the current `map_insert` insertion-pruning does — drifts only ~2.4e-8
113-
// here. An over-aggressive approximation that drops sub-threshold
114-
// contributions to *surviving* terms (the reverted iteration-5 behavior)
115-
// drifts ~2.2e-6 and trips this bound, which is the regression we guard.
110+
// truncation unit (1e-6) of exact. Standard magnitude truncation drifts
111+
// only ~2.4e-8 here. An over-aggressive approximation that drops
112+
// sub-threshold contributions to *surviving* terms drifts ~2.2e-6 and
113+
// trips this bound, which is the regression we guard.
116114
const TOL: f64 = 1e-6;
117115
let drift = (exact_val - approx_val).abs();
118116
assert!(
Lines changed: 79 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,79 @@
1+
// SPDX-FileCopyrightText: 2026 The PPVM Authors
2+
// SPDX-License-Identifier: Apache-2.0
3+
4+
//! Regression guard for `CoefficientThreshold` truncation semantics under
5+
//! *deferred* truncation.
6+
//!
7+
//! `PauliSum::map_insert` must never drop a gate-produced term on its own;
8+
//! terms are only ever removed by an explicit `truncate()`. A since-reverted
9+
//! optimization (the `Strategy::discard` insertion-pruning) skipped inserting
10+
//! a produced term whose coefficient was individually below the threshold and
11+
//! whose key was not yet present, on the assumption that the *next* call is
12+
//! always `truncate()`. That assumption breaks when a caller applies several
13+
//! gates between truncations: two sub-threshold contributions to the same key
14+
//! can sum to an above-threshold coefficient (`|a + b| ≥ τ` while
15+
//! `|a|, |b| < τ`), so dropping either at production loses a genuinely
16+
//! surviving term — and biases the terms it would have fed.
17+
//!
18+
//! This test pins the correct behavior: with truncation deferred across
19+
//! gates, the `CoefficientThreshold` result must equal a non-truncating
20+
//! reference filtered once at the end (standard "insert everything, truncate
21+
//! last" semantics).
22+
23+
use ppvm_runtime::prelude::*;
24+
use ppvm_runtime::strategy::CoefficientThreshold;
25+
26+
macro_rules! sorted_terms {
27+
($state:expr) => {{
28+
let mut v: Vec<(String, f64)> = $state.iter().map(|(k, c)| (k.to_string(), *c)).collect();
29+
v.sort_by(|a, b| a.0.cmp(&b.0));
30+
v
31+
}};
32+
}
33+
34+
#[test]
35+
fn deferred_truncation_keeps_terms_whose_merged_coefficient_survives() {
36+
// Two rx(θ) on a single Z with no truncate in between. The first rx
37+
// produces Y at ~sin(θ) ≈ 0.030 (< τ); the second adds another ~0.030, so
38+
// the accumulated Y ≈ sin(2θ) ≈ 0.060 is *above* τ and must survive the
39+
// final truncate. Insertion-pruning would drop the first (absent,
40+
// sub-threshold) Y and never accumulate it — losing the term entirely.
41+
const TAU: f64 = 0.05;
42+
const THETA: f64 = 0.03;
43+
44+
// CoefficientThreshold, truncation deferred until the very end.
45+
let mut approx: PauliSum<config::indexmap::ByteFxHashF64<2, CoefficientThreshold>> =
46+
PauliSum::builder()
47+
.n_qubits(1)
48+
.strategy(CoefficientThreshold(TAU))
49+
.build();
50+
approx += ("Z", 1.0);
51+
approx.rx(0, THETA);
52+
approx.rx(0, THETA);
53+
approx.truncate();
54+
55+
// Exact reference: NoStrategy never truncates; filter once at τ at the end.
56+
let mut exact: PauliSum<config::indexmap::ByteFxHashF64<2>> =
57+
PauliSum::builder().n_qubits(1).build();
58+
exact += ("Z", 1.0);
59+
exact.rx(0, THETA);
60+
exact.rx(0, THETA);
61+
let reference: Vec<(String, f64)> = sorted_terms!(exact)
62+
.into_iter()
63+
.filter(|(_, c)| c.abs() >= TAU)
64+
.collect();
65+
66+
// Sanity: the merged Y term really is above threshold (so this guards a
67+
// dropped surviving term, not just a coefficient nudge).
68+
assert!(
69+
reference.iter().any(|(k, c)| k == "Y" && c.abs() >= TAU),
70+
"test setup broken: reference should contain an above-threshold Y term, got {reference:?}"
71+
);
72+
73+
assert_eq!(
74+
sorted_terms!(approx),
75+
reference,
76+
"CoefficientThreshold with deferred truncation dropped or biased a term \
77+
that standard insert-then-truncate keeps"
78+
);
79+
}

0 commit comments

Comments
 (0)