diff --git a/docs/superpowers/plans/2026-08-27-filtered-estimates.md b/docs/superpowers/plans/2026-08-27-filtered-estimates.md new file mode 100644 index 0000000..a50e3b9 --- /dev/null +++ b/docs/superpowers/plans/2026-08-27-filtered-estimates.md @@ -0,0 +1,1285 @@ +# Filtered (Forward-Only) Estimates Implementation Plan + +> **For agentic workers:** REQUIRED SUB-SKILL: Use superpowers:subagent-driven-development (recommended) or superpowers:executing-plans to implement this plan task-by-task. Steps use checkbox (`- [ ]`) syntax for tracking. + +**Goal:** Replace the inert `HistoryBuilder::online(true)` with a working read-only forward-only pass, exposed as `filtered_log_evidence`, `filtered_learning_curves`, and `filtered_learning_curve`. + +**Architecture:** A pass walks `History::time_slices` in time order carrying its own `Index -> Gaussian` forward messages. Per slice it builds a scratch `TimeSlice` whose `backward` is left at `N_INF`, runs the *unmodified* production sweep (`iterate_to_convergence`) on that scratch, and harvests evidence and posteriors. Nothing is stored on `Skill` and nothing on `self` is mutated, so the results do not depend on whether `converge()` has run. + +**Tech Stack:** Rust 2024, no new dependencies. Features `approx` (numerical goldens) and `rayon` (parallel sweep) must both keep working. + +**Spec:** `docs/superpowers/specs/2026-08-27-filtered-estimates-design.md` — read it before Task 1. The plan argues from it; where they disagree, the spec wins and the plan is wrong. + +## Global Constraints + +- `#![forbid(unsafe_code)]` — no exceptions. +- MSRV is `rust-version = "1.85"` (Cargo.toml). **Let-chains push this to 1.88** and a CI job checks the floor — a previous commit had to rewrite two of them. Do not introduce any. +- Format with `cargo +nightly fmt` only. `rustfmt.toml` uses nightly-only options; stable `cargo fmt` silently does the wrong thing. +- **Run tests in release too.** `debug_assert!` is compiled out there and that is where defects in this crate have hidden. `just test` includes a release job. +- `pub(crate)` for internals; `pub` only at the crate API boundary. +- `#[cfg(test)] mod tests` goes at the END of a source file, never mid-file. +- Comments only where the *why* is non-obvious. No comments restating what the code does. +- Naming: short but descriptive. No single-character names except loop indices `i`/`j`/`k` and counts `n`. `err` not `e`, `posterior` not `p`. +- Blank line after every statement; multi-line `let` bindings always get a blank after. Adjacent `assert!`s stay grouped with no blank between them. +- `Skill`'s `backward` and `likelihood` fields are **module-private to `src/time_slice.rs`**. Any code constructing a `Skill` must live in that file. This is why the scratch builder is a `TimeSlice` method and not a `History` one. + +## Verification gates (every task) + +```bash +just check # fast inner loop: cargo test --features approx +just lint # clippy, warnings denied +just fmt # nightly +``` + +Before the final commit of the last task, also: + +```bash +just test # full feature matrix, includes the release job +just determinism # bit-identical posteriors at RAYON_NUM_THREADS 1/2/4/8 +``` + +## Test discipline (non-negotiable) + +**Prove every new test can fail.** Write it, break the production line it names, watch it go red for the *right* assertion, restore. A test never observed failing is not evidence, and reading it is not the check — reverting the line and re-running is. + +**A missing-symbol compile error is not a valid RED.** `cannot find method filtered_log_evidence` fires identically for a typo, a bad import, or a wrong signature; it proves nothing about the assertion. Where a task's red step needs a symbol that does not exist yet, the plan gives you a **stub encoding the current (buggy) behaviour** to add first, so the suite fails on an assertion with real numbers. Those stubs are deleted within the same task. + +**Restore every mutation before moving on, and prove you did.** A mutation +protocol interrupted midway leaves the tree in the *mutated* state, and the +usual "HEAD didn't advance, tree is clean" check does not catch it — the tree +is not clean, and a later `git add -A` would commit the sabotage. After each +mutate/observe/restore cycle, run `git diff --stat` and confirm it is empty +before the next step. Never commit while a mutation is in place. + +**The prescribed mutations may themselves be wrong.** Each one below is a +prediction about what would go red, made by reading rather than running. Two +mutations in an earlier draft of this plan were provably incapable of failing +their named tests. If a mutation does not go red, that is a finding about the +*test*, not a formality to wave through — report it. + +--- + +### Task 1: Remove the inert `online` machinery + +Pure deletion. `HistoryBuilder`'s `online` defaulted to `false` everywhere, so no existing value changes — the whole regression net must stay green, and that is this task's test. + +**Files:** +- Modify: `src/time_slice.rs:25`, `:41`, `:59-78`, `:108-124`, `:583-599`, `:626`, `:634` +- Modify: `src/history.rs:32`, `:63`, `:90-93`, `:138`, `:158`, `:174`, `:199`, `:226`, `:402`, `:410`, `:1183-1189` + +**Interfaces:** +- Consumes: nothing. +- Produces: `Skill` with exactly four fields (`forward`, `backward`, `likelihood`, `elapsed`); `Item::within_prior(forward, skills, agents)`; `Event::within_priors(forward, skills, agents)`; `TimeSlice::log_evidence(targets, forward, agents)`. Task 2 constructs `Skill` field-by-field and relies on that field list. + +- [ ] **Step 1: Record the baseline** + +The claim "no value changes" needs a before to compare against, not a plausible-looking after. + +```bash +cargo test --features approx 2>&1 | tail -5 > /tmp/baseline-task1.txt +cat /tmp/baseline-task1.txt +``` + +- [ ] **Step 2: Delete the `Skill.online` field** + +`src/time_slice.rs` — remove the field and its `Default` initialiser: + +```rust +#[derive(Debug)] +pub(crate) struct Skill { + pub(crate) forward: Gaussian, + backward: Gaussian, + likelihood: Gaussian, + pub(crate) elapsed: i64, +} + +impl Default for Skill { + fn default() -> Self { + Self { + forward: N_INF, + backward: N_INF, + likelihood: N_INF, + elapsed: 0, + } + } +} +``` + +- [ ] **Step 3: Drop the `online` parameter from `Item::within_prior`** + +`src/time_slice.rs` — the `if online` branch was the only reader of the deleted field: + +```rust +impl Item { + fn within_prior>( + &self, + forward: bool, + skills: &SkillStore, + agents: &CompetitorStore, + ) -> Rating { + let r = &agents[self.agent].rating; + let skill = skills.get(self.agent).unwrap(); + + if forward { + Rating::new(skill.forward, r.beta, r.drift) + } else { + Rating::new(skill.posterior() / self.likelihood, r.beta, r.drift) + } + } +} +``` + +- [ ] **Step 4: Drop the `online` parameter from `Event::within_priors`** + +`src/time_slice.rs`: + +```rust + pub(crate) fn within_priors>( + &self, + forward: bool, + skills: &SkillStore, + agents: &CompetitorStore, + ) -> Vec>> { + self.teams + .iter() + .map(|team| { + team.items + .iter() + .map(|item| item.within_prior(forward, skills, agents)) + .collect::>() + }) + .collect::>() + } +``` + +Two call sites also lose their leading `false`: `Event::compute` (`self.within_priors(false, false, ...)` becomes `self.within_priors(false, ...)`) and the sequential branch of `TimeSlice::iteration` (`event.within_priors(false, false, &self.skills, agents)` becomes `event.within_priors(false, &self.skills, agents)`). + +- [ ] **Step 5: Drop the `online` parameter from `TimeSlice::log_evidence`** + +`src/time_slice.rs` — signature loses the parameter, the `run_event` closure loses its argument, and both `online || forward` guards collapse to `forward`: + +```rust + pub(crate) fn log_evidence>( + &self, + targets: &[Index], + forward: bool, + agents: &CompetitorStore, + ) -> f64 { +``` + +Inside, `let teams = event.within_priors(online, forward, &self.skills, agents);` becomes `let teams = event.within_priors(forward, &self.skills, agents);`, `if online || forward {` becomes `if forward {`, and `} else if online || forward {` becomes `} else if forward {`. + +- [ ] **Step 6: Delete the builder flag and its copies** + +`src/history.rs` — remove `online: bool` from both the `HistoryBuilder` struct and the `History` struct, delete the `pub fn online` method entirely, and remove the field from all five struct literals (`drift()`, `observer()`, `build()`, `Default for HistoryBuilder`, `builder_with_key()`). + +```rust + pub fn online(mut self, online: bool) -> Self { + self.online = online; + self + } +``` + +...is deleted. Both `log_evidence_internal` call sites drop the argument: + +```rust + .map(|ts| ts.log_evidence(targets, forward, &self.agents)) +``` + +- [ ] **Step 7: Fix the misnamed test binding** + +`src/history.rs:1183` — the binding is called `..._online` but the flag it passes is `forward`, which is the confusion that let this bug survive. Rename it to say what it tests: + +```rust + let trueskill_log_evidence = h.log_evidence_internal(false, &[]); + let trueskill_log_evidence_forward = h.log_evidence_internal(true, &[]); + + assert_ulps_eq!( + trueskill_log_evidence, + trueskill_log_evidence_forward, + epsilon = 1e-6 + ); +``` + +- [ ] **Step 8: Verify nothing moved** + +```bash +cargo test --features approx 2>&1 | tail -5 +``` + +Expected: identical pass/fail counts to `/tmp/baseline-task1.txt`. Any numerical golden that *moves* here is a real finding — the deletion was supposed to be inert. Stop and investigate rather than re-blessing the golden. + +- [ ] **Step 9: Lint and format** + +```bash +just lint && just fmt +``` + +- [ ] **Step 10: Commit** + +```bash +git add src/time_slice.rs src/history.rs +git commit -m "refactor!: remove the inert online flag + +Skill.online was initialised to N_INF and assigned nowhere, so +HistoryBuilder::online(true) made every rating improper and log_evidence() +reported n * ln(0.5) — every game scored as a coin flip. The value is finite +and plausible, which is why it went unnoticed. + +The default was false, so no existing result changes. A working replacement +lands next; a stored field cannot hold the quantity, because converge() +alternates sweeps and contaminates skill.forward with backward information +from the second iteration onward. + +Also renames a test binding from ..._online to ..._forward: it passes the +forward flag, and the two senses being conflated is how this survived." +``` + +--- + +### Task 2: `filtered_log_evidence` + +The core. Scratch slice, forward pass, public method, and the red bracket test from the issue's own fixture. + +**Files:** +- Modify: `src/time_slice.rs` — add `FilteredStep` and `TimeSlice::filtered_step`; un-gate `iterate_to_convergence`; derive `Clone` on `Event`/`Team`/`Item` +- Modify: `src/history.rs` — add `filtered_pass` and `filtered_log_evidence` +- Create: `tests/filtered.rs` + +**Interfaces:** +- Consumes: `Skill { forward, backward, likelihood, elapsed }` from Task 1. +- Produces: `pub(crate) struct FilteredStep { log_evidence: f64, posteriors: Vec<(Index, Gaussian)> }`; `TimeSlice::filtered_step(&self, &HashMap, &CompetitorStore) -> FilteredStep`; `History::filtered_pass(&self) -> Vec<(T, FilteredStep)>`; `pub fn filtered_log_evidence(&self) -> f64`. Task 3 consumes `filtered_pass` and the `posteriors` field. Task 4 consumes `filtered_log_evidence`. + +- [ ] **Step 1: Write the failing test** + +Create `tests/filtered.rs`: + +```rust +//! Forward-only (filtering) estimates: what the model knew at the time, +//! as opposed to the smoothed posteriors `learning_curve` reports. + +use smallvec::smallvec; +use trueskill_tt::{Event, History, Member, Outcome, Team}; + +/// `games` one-on-one matches at successive times, won by "a" every time. +/// +/// This is the fixture from issue #19, where `online(true)` reported +/// `games * ln(0.5)`. +fn repeated_winner(games: i64) -> History { + let mut history = History::builder().build(); + + for time in 1..=games { + history + .add_events([Event { + time, + teams: smallvec![ + Team::with_members([Member::new("a")]), + Team::with_members([Member::new("b")]), + ], + outcome: Outcome::winner(0, 2), + }]) + .unwrap(); + } + + history +} + +#[test] +fn filtered_evidence_sits_between_coin_flip_and_batch() { + let mut history = repeated_winner(5); + + history.converge().unwrap(); + + let coin_flip = 5.0 * 0.5f64.ln(); + let batch = history.log_evidence(); + let filtered = history.filtered_log_evidence(); + + assert!( + filtered > coin_flip, + "filtered evidence {filtered} is at or below {coin_flip}, the all-coin-flip \ + value the inert online flag reported; game one is a coin flip but games two \ + through five are not" + ); + + assert!( + filtered < batch, + "filtered evidence {filtered} is not below the smoothed {batch}; filtering \ + scores each game on strictly less information than smoothing does" + ); +} +``` + +- [ ] **Step 2: Add the bug-encoding stub, so the red is an assertion and not a missing symbol** + +Add to `src/history.rs`, inside the main `impl, O: Observer, K: Eq + Hash + Clone> History` block. This reproduces exactly what `online(true)` used to return, so the suite fails on numbers rather than on a name: + +```rust + #[must_use] + pub fn filtered_log_evidence(&self) -> f64 { + self.time_slices + .iter() + .map(|slice| slice.events.len() as f64 * 0.5f64.ln()) + .sum() + } +``` + +- [ ] **Step 3: Run the test to verify it fails on the right assertion** + +```bash +cargo test --features approx --test filtered 2>&1 | tail -20 +``` + +Expected: FAIL on the *first* assertion — `filtered evidence -3.4657... is at or below -3.4657...`. That is the reported bug, reproduced as a test. If instead you see a compile error, the stub is wrong; fix it before continuing. If the *second* assertion is the one that fails, stop — the fixture is not behaving as the issue describes and the plan's premise is wrong. + +- [ ] **Step 4: Derive `Clone` on the event types** + +`src/time_slice.rs` — the scratch slice clones a slice's events: + +```rust +#[derive(Clone, Debug)] +struct Item { + agent: Index, + likelihood: Gaussian, +} + +#[derive(Clone, Debug)] +struct Team { + items: Vec, + output: f64, +} + +#[derive(Clone, Debug)] +pub(crate) struct Event { + teams: Vec, + log_evidence: f64, + weights: Vec>, + kind: EventKind, +} +``` + +- [ ] **Step 5: Un-gate `iterate_to_convergence` and correct its doc comment** + +`src/time_slice.rs:516` — commit `6030dc7` deliberately scoped this to `#[cfg(test)]` on the grounds that tests were all that used it. That stops being true here, and the comment saying so becomes load-bearing false prose. Delete the `#[cfg(test)]` attribute and replace the first doc paragraph: + +```rust + /// Iterate this slice alone until its posteriors stop moving, returning + /// the number of iterations taken. + /// + /// Used by `filtered_step` to drive a scratch copy of the slice, and by + /// tests. Production convergence across slices is driven by + /// `History::converge`, which calls `iteration` directly. + /// + /// Honours `self.convergence`; it previously hard-coded an epsilon and a + /// 20-iteration cap that matched neither `ConvergenceOptions` nor the + /// schedule default. + pub(crate) fn iterate_to_convergence>( +``` + +- [ ] **Step 6: Add `FilteredStep` and `TimeSlice::filtered_step`** + +`src/time_slice.rs`. Place `FilteredStep` next to `EventUpdate`, and `filtered_step` in the `impl TimeSlice` block after `new_forward_info`: + +```rust +/// One slice's worth of forward-only inference. +/// +/// `posteriors` doubles as the outgoing forward message: the scratch sweep +/// never writes `backward`, so it stays `N_INF`, and `Skill::posterior()` +/// and `forward_prior_out` are then the same product. +#[derive(Debug)] +pub(crate) struct FilteredStep { + pub(crate) log_evidence: f64, + pub(crate) posteriors: Vec<(Index, Gaussian)>, +} +``` + +```rust + /// Run this slice's events on forward (filtering) information alone. + /// + /// `incoming` holds each competitor's forward message out of their + /// previous appearance; a competitor absent from it starts at their + /// configured prior. The sweep runs on a scratch copy, so the real slice + /// is untouched — which is what makes the filtered estimates independent + /// of whether `History::converge` has run. + pub(crate) fn filtered_step>( + &self, + incoming: &HashMap, + agents: &CompetitorStore, + ) -> FilteredStep { + let mut scratch = TimeSlice { + events: self.events.clone(), + skills: SkillStore::new(), + time: self.time, + p_draw: self.p_draw, + convergence: self.convergence, + arena: ScratchArena::new(), + color_groups: ColorGroups::new(), + color_groups_dirty: true, + }; + + for event in &mut scratch.events { + for team in &mut event.teams { + for item in &mut team.items { + item.likelihood = N_INF; + } + } + + event.log_evidence = 0.0; + } + + for (agent, skill) in self.skills.iter() { + let rating = &agents[agent].rating; + + let forward = match incoming.get(&agent) { + Some(message) => message.forget(rating.drift.variance_for_elapsed(skill.elapsed)), + None => rating.prior, + }; + + scratch.skills.insert( + agent, + Skill { + forward, + backward: N_INF, + likelihood: N_INF, + elapsed: skill.elapsed, + }, + ); + } + + scratch.iterate_to_convergence(agents); + + FilteredStep { + log_evidence: scratch.events.iter().map(|event| event.log_evidence).sum(), + posteriors: scratch + .skills + .iter() + .map(|(agent, skill)| (agent, skill.posterior())) + .collect(), + } + } +``` + +Three details that are load-bearing: + +1. `Skill` is constructed field-by-field, **not** with `..Default::default()`. Adding a field to `Skill` later must be a compile error here, not a silent default that reads as a wrong answer. +2. `color_groups_dirty: true` makes `iterate_to_convergence` recompute the colour partition, so the scratch takes the same colour-group sweep the real slice does rather than the sequential fallback. +3. The `match incoming.get(&agent)` arms mirror `Competitor::receive_for_elapsed` (`src/competitor.rs:39`) exactly, including its fall back to the prior when no message exists. + +- [ ] **Step 7: Add the pass and replace the stub** + +`src/history.rs` — extend the existing import to `time_slice::{self, EventKind, FilteredStep, TimeSlice}`, then add both methods, deleting the Step 2 stub: + +```rust + /// Walk the slices in time order carrying forward messages only. + /// + /// This is the forward half of `iteration` with the backward half never + /// run. It reads `self` and mutates nothing. + fn filtered_pass(&self) -> Vec<(T, FilteredStep)> { + let mut messages: HashMap = HashMap::new(); + + let mut pass = Vec::with_capacity(self.time_slices.len()); + + for slice in &self.time_slices { + let step = slice.filtered_step(&messages, &self.agents); + + for &(agent, posterior) in &step.posteriors { + messages.insert(agent, posterior); + } + + pass.push((slice.time, step)); + } + + pass + } + + /// Total log-evidence under forward-only (filtering) information. + /// + /// Each event is scored using only what was known before it, which is the + /// right quantity for prequential scoring and model comparison. Contrast + /// `log_evidence`, whose per-event priors carry information from events + /// that had not happened yet. + /// + /// Runs a full forward pass per call and caches nothing. The result does + /// not depend on whether `converge` has been called. + #[must_use] + pub fn filtered_log_evidence(&self) -> f64 { + self.filtered_pass() + .iter() + .map(|(_, step)| step.log_evidence) + .sum() + } +``` + +- [ ] **Step 8: Run the test to verify it passes** + +```bash +cargo test --features approx --test filtered 2>&1 | tail -20 +``` + +Expected: PASS. Print the three numbers while you are here — they are quoted in the final report: + +```bash +cargo test --features approx --test filtered -- --nocapture 2>&1 | tail -20 +``` + +- [ ] **Step 9: Mutation-prove the test** + +The test has never been observed failing for the *right* reason with the real implementation in place. Make it: + +In `filtered_step`, change `backward: N_INF` to `backward: skill.backward`, reintroducing the contamination the spec argues against. + +```bash +cargo test --features approx --test filtered 2>&1 | tail -20 +``` + +Expected: one of the two assertions fails. Do not predict which — this +mutation is not cleanly "now it is smoothed": the scratch still resets every +likelihood to `N_INF` and still uses the carried forward message, and +`posterior()` no longer equals `forward_prior_out`, so the messages carried to +later slices become wrong in a second, independent way. The result is a hybrid. +Record which assertion failed and with what numbers. + +Restore `backward: N_INF`, re-run to confirm green, and check `git diff --stat` +is empty before continuing. + +- [ ] **Step 10: Full suite, lint, format** + +```bash +just check && just lint && just fmt +``` + +- [ ] **Step 11: Commit** + +```bash +git add src/time_slice.rs src/history.rs tests/filtered.rs +git commit -m "feat: add filtered_log_evidence + +Scores every event on what was known before it, rather than on priors that +carry information from events which had not happened yet. This is the +quantity HistoryBuilder::online promised and never delivered. + +The pass walks slices in time order carrying its own forward messages, and +per slice runs the unmodified production sweep on a scratch copy whose +backward message is left improper. Reusing iterate_to_convergence rather +than reimplementing inference means a competitor playing twice at one time +is handled by the same within-slice EP that converge() uses, instead of +being approximated the way the old evidence paths approximated it. + +Nothing is stored on Skill and nothing on self is mutated, so the result is +independent of whether converge() has run — the property a stored field +cannot have." +``` + +--- + +### Task 3: Filtered learning curves + +**Files:** +- Modify: `src/history.rs` — add `filtered_learning_curves` and `filtered_learning_curve` +- Modify: `tests/filtered.rs` + +**Interfaces:** +- Consumes: `History::filtered_pass() -> Vec<(T, FilteredStep)>` and `FilteredStep::posteriors` from Task 2. +- Produces: `pub fn filtered_learning_curves(&self) -> HashMap>`; `pub fn filtered_learning_curve(&self, key: &Q) -> Vec<(T, Gaussian)>`. Task 4 consumes both. + +- [ ] **Step 1: Write the failing test** + +Append to `tests/filtered.rs`. This is the ustat complaint stated as an assertion — smoothed curves start already collapsed, filtered ones do not: + +```rust +#[test] +fn filtered_first_point_is_less_certain_than_smoothed() { + let mut history = repeated_winner(12); + + history.converge().unwrap(); + + let smoothed = history.learning_curve("a"); + let filtered = history.filtered_learning_curve("a"); + + assert_eq!( + smoothed.len(), + filtered.len(), + "both curves must cover the same time points" + ); + + let (smoothed_time, first_smoothed) = smoothed[0]; + let (filtered_time, first_filtered) = filtered[0]; + + assert_eq!(smoothed_time, filtered_time); + + assert!( + first_filtered.sigma() > first_smoothed.sigma(), + "filtered sigma {} at the first point is not above smoothed {}; the smoother \ + collapses uncertainty before the first round is drawn, which is the whole \ + reason this method exists", + first_filtered.sigma(), + first_smoothed.sigma() + ); + + assert!( + first_filtered.sigma() < trueskill_tt::SIGMA, + "filtered sigma {} at the first point is not below the prior {}; one game was \ + played, so some uncertainty must have been resolved", + first_filtered.sigma(), + trueskill_tt::SIGMA + ); + + for pair in filtered.windows(2) { + assert!( + pair[1].1.mu() > pair[0].1.mu(), + "filtered mu must climb at every step for a competitor who wins every \ + game: t={} mu={} then t={} mu={}", + pair[0].0, + pair[0].1.mu(), + pair[1].0, + pair[1].1.mu() + ); + } +} +``` + +The monotonicity loop is what makes this test reachable by a mutation to the +*carried messages*. The first two assertions only inspect `filtered[0]`, which +is produced from an empty message map and so is identical under any mutation to +what `filtered_pass` writes *after* a slice. Without the loop this test cannot +detect a broken carry-forward at all. + +If monotonicity fails at some interior point against the *correct* +implementation, stop and report it rather than weakening the assertion. For +twelve straight wins, mu climbing at every step is a real property, and a +violation is a finding about the inference, not about the test. + +- [ ] **Step 2: Add the second failing test, for the plural form** + +Without this, `filtered_learning_curves` is asserted only by +`assert!(...is_empty())` on an empty history (Task 4) — which passes for an +implementation that always returns an empty map. Append to `tests/filtered.rs`: + +```rust +#[test] +fn filtered_curves_plural_agrees_with_singular() { + let mut history = repeated_winner(4); + + history.converge().unwrap(); + + let curves = history.filtered_learning_curves(); + + assert_eq!( + curves["b"], + history.filtered_learning_curve("b"), + "the plural form must agree with the singular for the same key" + ); +} +``` + +Key `"b"` rather than `"a"` on purpose: `"a"` is `Index(0)`, so a singular +implementation that returned whichever competitor came first would still look +correct for `"a"`. `Gaussian` derives `PartialEq` and `Debug` +(`src/gaussian.rs:13`), so `assert_eq!` over `Vec<(i64, Gaussian)>` compiles. + +- [ ] **Step 3: Add the stub encoding current behaviour** + +`src/history.rs` — returning the *smoothed* curve is the honest "filtering not implemented" behaviour, and makes the red an assertion with real sigmas: + +```rust + pub fn filtered_learning_curves(&self) -> HashMap> { + self.learning_curves() + } + + pub fn filtered_learning_curve(&self, key: &Q) -> Vec<(T, Gaussian)> + where + K: Borrow, + Q: Hash + Eq + ?Sized, + { + self.learning_curve(key) + } +``` + +Both stubs are needed: without the plural one, Step 2's test fails to compile +and Step 4 gets a missing-symbol error instead of an assertion. + +- [ ] **Step 4: Run the tests to verify they fail on the right assertions** + +```bash +cargo test --features approx --test filtered filtered_first_point 2>&1 | tail -20 +``` + +Expected: FAIL on `filtered sigma X at the first point is not above smoothed X` — the two are identical because the stub returns the same curve. Not a compile error. + +- [ ] **Step 5: Implement both methods** + +`src/history.rs` — replace the stub. Place these next to `learning_curve` / `learning_curves` so the smoothed and filtered pairs read together: + +```rust + /// Filtered learning curves for all competitors, keyed by user-facing key. + /// + /// Each point is the posterior using only events up to and including that + /// time — "what we knew then". Contrast `learning_curves`, whose points + /// are smoothed and so incorporate rounds played later. + /// + /// Runs a full forward pass per call and caches nothing. + pub fn filtered_learning_curves(&self) -> HashMap> { + let mut data: HashMap> = HashMap::new(); + + for (time, step) in self.filtered_pass() { + for (agent, posterior) in step.posteriors { + if let Some(key) = self.keys.key(agent).cloned() { + data.entry(key).or_default().push((time, posterior)); + } + } + } + + data + } + + /// Filtered learning curve for a single key: (time, posterior) pairs in + /// time order. + /// + /// Runs the same full pass as `filtered_learning_curves` and keeps one + /// key, so asking for several keys individually costs a pass each — use + /// the plural form for that. + pub fn filtered_learning_curve(&self, key: &Q) -> Vec<(T, Gaussian)> + where + K: Borrow, + Q: Hash + Eq + ?Sized, + { + let Some(idx) = self.keys.get(key) else { + return Vec::new(); + }; + + self.filtered_pass() + .into_iter() + .filter_map(|(time, step)| { + step.posteriors + .iter() + .find(|(agent, _)| *agent == idx) + .map(|&(_, posterior)| (time, posterior)) + }) + .collect() + } +``` + +- [ ] **Step 6: Correct the stale complexity note on `learning_curves`** + +Do not copy `learning_curves`' doc comment. `src/history.rs:323-324` says: + +```rust + /// Note: `key(idx)` is O(n) per lookup; this method is therefore O(n²) + /// in the number of competitors. Acceptable for T2; T3 may optimize. +``` + +That is false. `KeyTable::key` is `self.reverse.get(idx.0)` (`src/key_table.rs:57-59`) +— an O(1) `Vec` index — and the type's own doc at `src/key_table.rs:19-21` says +so: *"position \*is\* the index and `key()` is a lookup rather than a scan"*. +The note predates `reverse` becoming a `Vec`. Delete those two lines from +`learning_curves`; the spec's whole argument turns on prose that is quietly +false, so leaving a known-false comment in place while citing that risk would +be indefensible. + +- [ ] **Step 7: Run the tests to verify they pass** + +```bash +cargo test --features approx --test filtered 2>&1 | tail -20 +``` + +Expected: PASS, all three tests. + +- [ ] **Step 8: Mutation-prove both tests** + +Two mutations, one per test, restoring between them. + +**Mutation A — freeze the carried messages.** In `filtered_pass`, guard the +carry-forward so each competitor keeps only their first message: + +```rust + for &(agent, posterior) in &step.posteriors { + if messages.contains_key(&agent) { + continue; + } + + messages.insert(agent, posterior); + } +``` + +```bash +cargo test --features approx --test filtered filtered_first_point 2>&1 | tail -20 +``` + +Expected: FAIL **on the monotonicity loop**, not on either sigma assertion — +with messages frozen, every slice after the first runs from the same incoming +prior, so mu plateaus instead of climbing. If it fails on a sigma assertion +instead, something other than the carry-forward changed; investigate. + +**Mutation B — return the wrong competitor from the singular form.** In +`filtered_learning_curve`, change `.find(|(agent, _)| *agent == idx)` to +`.find(|_| true)`. + +```bash +cargo test --features approx --test filtered filtered_curves_plural 2>&1 | tail -20 +``` + +Expected: FAIL — the singular form now returns `"a"`'s curve while the plural +form still keys `"b"` correctly. + +Restore both, re-run to confirm green, and check `git diff --stat` is empty. + +- [ ] **Step 9: Lint, format, commit** + +```bash +just check && just lint && just fmt +git add src/history.rs tests/filtered.rs +git commit -m "feat: add filtered learning curves + +learning_curve returns post-convergence posteriors, so every point is +smoothed: the estimate at a given date incorporates rounds played years +later. On ustat's data that starts six players' curves already spread apart +at sigma 0.9-1.6 against a prior of 6.0, barely moving thereafter. + +filtered_learning_curve plots the same competitor on forward-only +information, so everyone starts at the prior and fans out. It could not be +reconstructed from the public API before: a caller could only refit over +events[0..k] for every k, which is O(n^2) fits for something one forward +pass already computes." +``` + +--- + +### Task 4: The invariants + +The bracket test proves the feature works on one fixture. These prove the *bug class* is gone. + +**Files:** +- Modify: `tests/filtered.rs` +- Modify: `tests/degenerate_inputs.rs` + +**Interfaces:** +- Consumes: `filtered_log_evidence`, `filtered_learning_curve`, `filtered_learning_curves` from Tasks 2 and 3. +- Produces: no new symbols. + +- [ ] **Step 1: Add the tight-convergence helper and the converge-invariance test** + +Append to `tests/filtered.rs`. Agreement is to tolerance, not bit-identity — `iteration` recomputes the colour partition only when `from == 0`, so an incrementally-built slice keeps insertion order until the first `converge()` reorders it, and the scratch clone inherits whichever order it finds. Same fixed point, different path. This follows the house pattern in `tests/ingestion_equivalence.rs`: + +Extend the existing top-of-file `use` in `tests/filtered.rs` to +`use trueskill_tt::{ConvergenceOptions, Event, History, Member, Outcome, Team};` +— `cargo +nightly fmt` will not merge a second `use` added mid-file, so it +would survive as a wart. + +```rust +/// The default 30-iteration cap leaves a residual around 1e-6, which would +/// swamp these comparisons. Drive both sides well past the fixed point. +fn tight() -> ConvergenceOptions { + ConvergenceOptions { + max_iter: 2_000, + epsilon: 1e-12, + ..ConvergenceOptions::default() + } +} + +fn repeated_winner_tight(games: i64) -> History { + let mut history = History::builder().convergence(tight()).build(); + + for time in 1..=games { + history + .add_events([Event { + time, + teams: smallvec![ + Team::with_members([Member::new("a")]), + Team::with_members([Member::new("b")]), + ], + outcome: Outcome::winner(0, 2), + }]) + .unwrap(); + } + + history +} + +#[test] +fn filtered_evidence_is_invariant_to_convergence() { + let mut history = repeated_winner_tight(6); + + let before = history.filtered_log_evidence(); + + let report = history.converge().unwrap(); + assert!(report.converged, "fixture must converge: {:?}", report.final_step); + + let after = history.filtered_log_evidence(); + + assert!( + (before - after).abs() < 1e-8, + "filtered evidence moved across converge(): {before} -> {after}. The pass must \ + carry its own forward messages; anything reading skill.forward shows exactly \ + this drift, because converge() contaminates it with backward information." + ); +} +``` + +- [ ] **Step 2: Run it** + +```bash +cargo test --features approx --test filtered filtered_evidence_is_invariant 2>&1 | tail -20 +``` + +Expected: PASS. + +- [ ] **Step 3: Mutation-prove it — this is the most important mutation in the plan** + +Make `filtered_step` read the real slice's forward message instead of the carried one, which is the fix the issue originally proposed: + +```rust + let forward = skill.forward; +``` + +...replacing the whole `match incoming.get(&agent)` expression. + +```bash +cargo test --features approx --test filtered filtered_evidence_is_invariant 2>&1 | tail -20 +``` + +Expected: FAIL, with `before` and `after` visibly different. **This failure is the evidence that the spec's central argument is true.** Record the two numbers — they belong in the final report and in the issue comment. Then restore. + +- [ ] **Step 4: Add the single-slice exactness test** + +With one time slice there is no backward sweep to run (`iteration`'s backward loop is empty when `len == 1`), so filtered and smoothed must agree: + +```rust +#[test] +fn single_slice_filtered_matches_smoothed() { + let mut history = History::builder().convergence(tight()).build(); + + history + .add_events([ + Event { + time: 1, + teams: smallvec![ + Team::with_members([Member::new("a")]), + Team::with_members([Member::new("b")]), + ], + outcome: Outcome::winner(0, 2), + }, + Event { + time: 1, + teams: smallvec![ + Team::with_members([Member::new("c")]), + Team::with_members([Member::new("d")]), + ], + outcome: Outcome::winner(0, 2), + }, + ]) + .unwrap(); + + history.converge().unwrap(); + + let smoothed = history.learning_curve("a"); + let filtered = history.filtered_learning_curve("a"); + + assert_eq!(smoothed.len(), 1); + assert_eq!(filtered.len(), 1); + + assert!( + (smoothed[0].1.mu() - filtered[0].1.mu()).abs() < 1e-8 + && (smoothed[0].1.sigma() - filtered[0].1.sigma()).abs() < 1e-8, + "one slice has no future to propagate back, so filtered and smoothed must \ + agree: smoothed mu={} sigma={}, filtered mu={} sigma={}", + smoothed[0].1.mu(), + smoothed[0].1.sigma(), + filtered[0].1.mu(), + filtered[0].1.sigma() + ); +} +``` + +- [ ] **Step 5: Add the ingestion-order invariance test** + +The crate's standing invariant is that batching must not change the answer; the +filtered pass must honour it too. + +**The fixture shape is the whole test.** Four events at four *distinct* +timestamps — the obvious choice — yields one event per slice, which makes the +scratch's event order trivially identical on both paths and the assertion true +by construction. `tests/ingestion_equivalence.rs:81` documents the shape that +actually exercises the append-to-an-existing-slice path: *"All events share one +timestamp, so incremental ingestion repeatedly appends"*. Use several events per +timestamp, with competitors shared between events in the same slice so the +colour partition is non-trivial: + +```rust +#[test] +fn filtered_curves_do_not_depend_on_ingestion_order() { + let events = |time: i64, winner: &'static str, loser: &'static str| Event { + time, + teams: smallvec![ + Team::with_members([Member::new(winner)]), + Team::with_members([Member::new(loser)]), + ], + outcome: Outcome::winner(0, 2), + }; + + let all = vec![ + events(1, "a", "b"), + events(1, "c", "d"), + events(1, "a", "c"), + events(1, "b", "d"), + events(2, "a", "d"), + events(2, "b", "c"), + events(2, "a", "b"), + ]; + + let mut batched = History::builder().convergence(tight()).build(); + batched.add_events(all.clone()).unwrap(); + batched.converge().unwrap(); + + let mut incremental = History::builder().convergence(tight()).build(); + for event in all { + incremental.add_events([event]).unwrap(); + } + incremental.converge().unwrap(); + + let from_batched = batched.filtered_learning_curve("a"); + let from_incremental = incremental.filtered_learning_curve("a"); + + assert_eq!(from_batched.len(), from_incremental.len()); + + for ((time_b, gaussian_b), (time_i, gaussian_i)) in + from_batched.iter().zip(from_incremental.iter()) + { + assert_eq!(time_b, time_i); + + assert!( + (gaussian_b.mu() - gaussian_i.mu()).abs() < 1e-8 + && (gaussian_b.sigma() - gaussian_i.sigma()).abs() < 1e-8, + "at t={time_b}: batched mu={} sigma={}, incremental mu={} sigma={}", + gaussian_b.mu(), + gaussian_b.sigma(), + gaussian_i.mu(), + gaussian_i.sigma() + ); + } +} +``` + +- [ ] **Step 6: Add the degenerate cases** + +Append to `tests/degenerate_inputs.rs`, matching whatever import style that file already uses: + +```rust +#[test] +fn empty_history_has_no_filtered_estimates() { + let history: History = History::builder().build(); + + assert_eq!(history.filtered_log_evidence(), 0.0); + + assert!(history.filtered_learning_curves().is_empty()); + + assert!(history.filtered_learning_curve("nobody").is_empty()); +} +``` + +- [ ] **Step 7: Run everything** + +```bash +just check +``` + +Expected: all green. + +- [ ] **Step 8: Mutation-prove the remaining three** + +One at a time, restoring between each, with `git diff --stat` empty before +moving on: + +- *single-slice exactness*: in `filtered_step`, change `None => rating.prior` to `None => rating.prior.forget(1.0)`. Expected: this test fails on sigma. +- *degenerate*: in `filtered_log_evidence`, change `.sum()` to `.sum::() + 1.0`. Expected: the empty-history assertion fails. + +The ingestion-order test gets **no prescribed mutation**, deliberately. Any +mutation to `filtered_step` breaks the batched and incremental paths equally, so +it cannot flip an equality between them. Its value is as a regression guard for +the crate's standing invariant, not as a mutation-proved assertion, and claiming +otherwise would be the vacuous-coverage failure this plan warns about. + +- [ ] **Step 9: Run the colour-partition experiment and report the result** + +The spec claims filtered results agree only to tolerance, not bit-identically, +because the scratch inherits whichever event order the real slice happens to be +in. That claim is untested. Find out: + +```bash +cargo test --features approx --test filtered filtered_curves_do_not_depend 2>&1 | tail -20 +``` + +...then change `color_groups_dirty: true` to `false` in `filtered_step` and +re-run the same test. + +This is an **experiment, not a mutation proof** — either outcome is a valid +result and both must be reported: + +- If it goes red, the colour partition genuinely affects the scratch sweep, the + spec's tolerance argument is confirmed, and the fixture is strong enough. +- If it stays green, the concern is not reachable even by this fixture. Say so. + It would mean the spec's *Risks* entry overstates the problem and the + tolerance-not-bit-identity caveat may be unnecessary — worth an issue, not a + silent shrug. + +Restore `color_groups_dirty: true` either way and confirm `git diff --stat` is +empty. + +- [ ] **Step 10: Commit** + +```bash +git add tests/filtered.rs tests/degenerate_inputs.rs +git commit -m "test: pin the invariants that make filtered estimates trustworthy + +The bracket test proves the feature works on one fixture. These pin the bug +class: + +- Invariance to converge(). This is the one that matters. Reading + skill.forward instead of the carried message makes it fail immediately, + because converge() alternates sweeps and contaminates skill.forward with + backward information from the second iteration onward. That is the + property a stored field cannot have, and the reason issue #19's proposed + fix would not have worked. +- Invariance to ingestion order, the crate's standing invariant. +- One slice has no future to propagate back, so filtered equals smoothed. +- Empty history yields zero and empty maps. + +Agreement is to 1e-8 under tight convergence rather than bit-identity: +iteration recomputes the colour partition only when from == 0, so an +incrementally built slice keeps insertion order until the first converge() +reorders it, and the scratch clone inherits whichever order it finds. Same +fixed point, different path to it." +``` + +--- + +### Task 5: Record the API break, close out + +**Files:** +- Modify: `src/lib.rs` (crate docs, if they mention evidence or learning curves) +- Modify: `README.md` (if it lists the public API) +- Verify: `CHANGELOG.md` regeneration + +**Interfaces:** +- Consumes: everything from Tasks 1-4. +- Produces: no new symbols. + +- [ ] **Step 1: Check what the generated changelog will say** + +`CHANGELOG.md` is **generated by git-cliff from commit messages** (`cliff.toml`, and commit `9506fed` fixed its output location). Hand-editing it would be clobbered at the next release, so the API break has to be recorded through the commit subjects — which is why Task 1 used `refactor!:`. + +```bash +git cliff --unreleased 2>&1 | head -40 +``` + +Read the output. Confirm the `refactor!: remove the inert online flag` entry appears and that the break is visible. + +- [ ] **Step 2: If the break is not visible, report it — do not paper over it** + +`cliff.toml`'s `commit_parsers` match on `^feat`, `^fix`, `^doc`, and so on, with **no breaking-change parser**. If `git cliff` renders the `!` invisibly or files the commit under a plain "Refactor" heading with nothing marking the break, that is a real gap in the release tooling. + +Do **not** hand-edit `CHANGELOG.md` to compensate — it regenerates. Instead, note the finding for the final report and propose (without applying, as it is outside this plan's scope) adding a breaking-change group to `cliff.toml`. + +- [ ] **Step 3: Check whether the crate docs need updating** + +```bash +grep -n "online\|log_evidence\|learning_curve" src/lib.rs README.md +``` + +If `src/lib.rs`'s `//!` header or `README.md` lists the public API or documents evidence, add the three `filtered_*` methods alongside their smoothed counterparts. If neither mentions them, change nothing — do not invent documentation the task did not ask for. + +Note that `src/lib.rs` doc examples are checked by `cargo test --doc`, so any example you add must actually run. + +- [ ] **Step 4: Run the full gate** + +```bash +just test +``` + +Expected: green across every feature combination, **including the release job**. `debug_assert!` is compiled out in release and that is where defects in this crate have hidden before; a debug-only pass is not evidence. + +- [ ] **Step 5: Run the determinism gate** + +```bash +just determinism +``` + +Expected: bit-identical posteriors at `RAYON_NUM_THREADS` 1/2/4/8. The scratch sweep goes through the same `sweep_color_groups` the real one does, so this should be unaffected — but it is the gate that would catch it if the scratch construction perturbed apply order. + +- [ ] **Step 6: Confirm the issue's acceptance criteria** + +Issue #19 asks for either a correct filtering log-evidence with a test that would fail against the all-`N_INF` behaviour, or the setting gone from the public API. This plan does both. Re-read the issue and the spec's *Acceptance* framing, and write down for each criterion which test or commit satisfies it. + +- [ ] **Step 7: Commit any doc changes** + +```bash +git add -A +git commit -m "docs: document the filtered estimates in the crate header" +``` + +Skip this commit entirely if Step 3 found nothing to change. An empty documentation commit is noise. + +- [ ] **Step 8: File the two follow-ups** + +The spec defers these deliberately; they should not evaporate: + +1. `log_evidence_internal`'s `forward: bool` (`src/history.rs:395`) is a filtering quantity only on a history that was never converged. Either document the constraint or fold the flag into the new pass and delete it. +2. `log_evidence` (`src/history.rs:416`) takes `&mut self` but mutates nothing; the new `filtered_*` methods take `&self`. + +Both go to `logaritmisk/trueskill-tt` on `git.aceofba.se`. + +- [ ] **Step 9: Report back** + +Summarise for the human partner: + +- the three numbers from Task 2 Step 8 (coin-flip bound, filtered, batch); +- which assertion the Task 2 Step 9 mutation flipped, and with what numbers; +- the before/after pair from the Task 4 Step 3 mutation — this is the evidence + that the spec's central argument holds; +- the outcome of the Task 4 Step 9 colour-partition experiment, red or green; +- whatever Task 5 Step 1 found about the generated changelog; +- any mutation that did not go red as predicted. + +Do not merge — `finishing-a-development-branch` is a separate decision for the +human partner. + +--- + +## Self-review + +**Spec coverage.** Every numbered item in the spec's *What ships* maps to a task: the pass and its scratch step to Task 2, the three public methods to Tasks 2 and 3, the removal inventory to Task 1, the `Clone` derives and the `iterate_to_convergence` un-gating to Task 2 Steps 4-5, the changelog record to Task 5. Every invariant in the spec's *Testing strategy* maps to a step in Task 4, and the red bracket test to Task 2 Step 1. + +**Two spec items are handled differently than written**, and the plan's +"where they disagree, the spec wins" rule does *not* apply to either: + +1. The spec says "A CHANGELOG entry recording the API break". `CHANGELOG.md` is + generated by git-cliff, so Task 5 records the break through the commit + subject (`refactor!:`) and *verifies* the generated output rather than + hand-writing an entry that would be overwritten at the next release. This is + the spec's intent, not its letter. +2. The spec's *Design* section now declares `filtered_pass` as + `Vec<(T, FilteredStep)>`. An earlier draft declared it as + `Vec<(T, Vec<(Index, Gaussian)>)>`, which cannot carry the evidence its own + step 3 harvests. If you are reading a spec revision with the older + signature, the plan is right and the spec is wrong. + +**Reviewed independently.** An adversarial reviewer checked this plan against +the source and found two prescribed mutations that provably could not fail +their named tests, a near-vacuous ingestion-order fixture, an unasserted public +method, and a doc comment restating a false O(n²) claim. All are fixed above. +The reviewer confirmed the central correctness claim by tracing the call graph: +`N_INF` is `{pi: 0, tau: 0}` and `Mul` is a natural-parameter add, so it is an +exact multiplicative identity; and the only write to `skill.backward` in the +crate is at `src/time_slice.rs:571` inside `new_backward_info`, reachable only +from `History::iteration` and never from `iterate_to_convergence` under either +`rayon` cfg. + +**Known weak points, stated rather than hidden.** + +- The direction assertion `filtered < batch` (Task 2 Step 1) is a strong + expectation about EP behaviour, not a proof. If it fails, the fixture or the + reasoning is wrong — investigate rather than flipping the comparison. +- Nothing in this plan has been compiled. Every claim that a code block builds + comes from reading declarations, not from `cargo check`. Expect to fix small + type and borrow errors as you go; treat a *large* one as a signal that the + plan misread something and say so. +- The Task 3 monotonicity assertion assumes mu climbs at every step across + twelve straight wins. That is a real property, but it has not been measured. diff --git a/docs/superpowers/specs/2026-08-27-filtered-estimates-design.md b/docs/superpowers/specs/2026-08-27-filtered-estimates-design.md index 14bcf16..9015775 100644 --- a/docs/superpowers/specs/2026-08-27-filtered-estimates-design.md +++ b/docs/superpowers/specs/2026-08-27-filtered-estimates-design.md @@ -125,9 +125,18 @@ the present bug reads as plausible. ### The pass ```rust -fn filtered_pass(&self) -> Vec<(T, Vec<(Index, Gaussian)>)> +pub(crate) struct FilteredStep { + log_evidence: f64, + posteriors: Vec<(Index, Gaussian)>, +} + +fn filtered_pass(&self) -> Vec<(T, FilteredStep)> ``` +`posteriors` doubles as the outgoing forward message: the scratch sweep never +writes `backward`, so it stays `N_INF`, and `Skill::posterior()` and +`forward_prior_out` are then the same product. + Walk `self.time_slices` in order, carrying `messages: HashMap` — the forward message out of each competitor's most recent appearance. For each slice: @@ -146,7 +155,7 @@ competitor's most recent appearance. For each slice: elapsed = skill.elapsed // copied from the real slice ``` - This mirrors `Competitor::receive_for_elapsed` (`src/competitor.rs:38`) + This mirrors `Competitor::receive_for_elapsed` (`src/competitor.rs:39`) exactly, including its `message != N_INF` fallback to the prior. `skill.elapsed` is reused rather than recomputed: it is maintained by `add_events_with_prior` across out-of-order ingestion, and production @@ -211,7 +220,7 @@ is the property a stored field cannot have, and it is asserted as a test. | `src/time_slice.rs:110,120` | drop `online` param from `Event::within_priors` | | `src/time_slice.rs:585,597,626,634` | drop `online` param from `TimeSlice::log_evidence`; `online \|\| forward` becomes `forward` | | `src/history.rs:32,63,138,158,174,199,226` | delete the two `online` field declarations (`:32`, `:199`) and the five struct-literal copies | -| `src/history.rs:90-92` | delete `HistoryBuilder::online()` | +| `src/history.rs:90-93` | delete `HistoryBuilder::online()` | | `src/history.rs:402,410` | drop the `self.online` argument | | `src/history.rs:1183-1189` | the `..._online` assertion becomes a `forward`-flag assertion; rename the binding to match what it tests | @@ -242,9 +251,20 @@ coin flip under filtering, games two through five are not. ### Invariants 1. **Invariant to `converge()`** — `filtered_log_evidence()` and - `filtered_learning_curves()` are bit-identical before and after - `converge()`. This is exactly what `skill.forward` fails, and what - makes a stored field the wrong mechanism. + `filtered_learning_curves()` agree before and after `converge()`. This + is exactly what `skill.forward` fails, and what makes a stored field + the wrong mechanism. + + Agreement is to tolerance, not bit-identity, and the reason is worth + recording. `iteration` calls `recompute_color_groups` + (`src/time_slice.rs:369`) only when `from == 0`, so a slice built by + repeated appends keeps insertion order until the first `converge()` + reorders it. The scratch clone inherits whichever order it finds, and + greedy coloring over a permuted input can group differently, giving a + different within-slice sweep order — same EP fixed point, different + path to it. Follow the house pattern in + `tests/ingestion_equivalence.rs`: converge tightly (`max_iter: 2_000`, + `epsilon: 1e-12`) and compare within `1e-8`. 2. **Invariant to ingestion order** — events added one at a time produce the same filtered results as the same events batched. Extends the existing invariant in `tests/ingestion_equivalence.rs`. @@ -286,6 +306,12 @@ inert. - **`iterate_to_convergence` leaving test-only status.** Its doc comment claims "only used by tests"; that comment must be updated, or it becomes the next piece of load-bearing prose that is quietly false. +- **Event order is inherited, not normalised.** The scratch clone takes + the real slice's current event order, which differs pre- and + post-`converge()` for incrementally-ingested slices (see *Invariants*). + Results agree to within convergence tolerance rather than exactly. + Normalising the order in the scratch builder would buy bit-identity at + the cost of diverging from what the real sweep does; not worth it. - **Divergence risk.** If `TimeSlice`'s sweep gains state that the scratch construction does not initialise, the pass silently reads a default. The scratch builder must construct `Skill` field-by-field