From 187aede924fec246b59c277ca6f3e12b444c2b61 Mon Sep 17 00:00:00 2001 From: Anders Olsson Date: Thu, 27 Aug 2026 16:04:16 +0200 Subject: [PATCH] docs: implementation plan for filtered estimates MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Five tasks: delete the inert online machinery, add filtered_log_evidence, add the two learning-curve methods, pin the invariants, record the API break. Two spec corrections fell out of writing it. The spec claimed filtered results would be bit-identical before and after converge(); they cannot be. iteration recomputes the colour partition only when from == 0, so a slice built by repeated appends keeps insertion order until the first converge() reorders it, and the scratch clone inherits whichever order it finds — same fixed point, different path. Corrected to agreement within 1e-8 under tight convergence, matching the house pattern in tests/ingestion_equivalence.rs. The spec also declared filtered_pass as Vec<(T, Vec<(Index, Gaussian)>)>, which cannot carry the evidence its own step 3 harvests; it returns Vec<(T, FilteredStep)>. CHANGELOG.md is generated by git-cliff, so the spec's "CHANGELOG records the API break" cannot be satisfied by editing the file — it regenerates. Task 5 records the break through the commit subject and verifies the generated output instead. cliff.toml has no breaking-change parser at all, which the task is told to report rather than work around. An adversarial reviewer checked the plan against the source before this commit and found four real defects, all in plan text, none in the design: - Two prescribed mutations provably could not fail their named tests. The learning-curve mutation altered only what filtered_pass writes after a slice, while the test inspected filtered[0], which is computed from an empty message map. Fixed by asserting monotonic mu across the whole curve. - The ingestion-order fixture used four distinct timestamps, giving one event per slice — the exact degenerate shape ingestion_equivalence.rs documents as the weak case, making the assertion true by construction. Fixed to several events per timestamp with shared competitors. - filtered_learning_curves was never asserted for content, only for emptiness on an empty history. - A doc comment restated learning_curves' claim that key(idx) is O(n) and the method O(n^2). KeyTable::key is self.reverse.get(idx.0) — O(1) — and the type's own doc says so. The claim predates reverse becoming a Vec. The plan now corrects the original at history.rs:323 rather than copying it. The reviewer confirmed the central 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 in new_backward_info, reachable only from History::iteration and never from iterate_to_convergence under either rayon cfg. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01T5SYDExxL4vZgvunrcNSMc --- .../plans/2026-08-27-filtered-estimates.md | 1285 +++++++++++++++++ .../2026-08-27-filtered-estimates-design.md | 38 +- 2 files changed, 1317 insertions(+), 6 deletions(-) create mode 100644 docs/superpowers/plans/2026-08-27-filtered-estimates.md 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