From 4fde482e481b44f8609ebc809fd5f0b8fcfa23fe Mon Sep 17 00:00:00 2001 From: Anders Olsson Date: Thu, 27 Aug 2026 15:42:54 +0200 Subject: [PATCH] docs: spec for filtered (forward-only) estimates MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `HistoryBuilder::online(true)` is inert: it flips a flag that reaches `Item::within_prior`, which reads `Skill.online` — a field initialised to `N_INF` and assigned nowhere. So `log_evidence()` under that setting reports `n * ln(0.5)`, every game scored as a coin flip. The number is finite and plausible, which is why nothing caught it. Issue #19 proposed populating the field during the forward pass. That does not work, and the reason shapes the whole design. `new_forward_info` sets `skill.forward` from the previous slice's `forward_prior_out`, which is `skill.forward * skill.likelihood`; `History::iteration` alternates backward and forward sweeps, so from the second iteration onward that likelihood has already absorbed backward information. After `converge()`, `skill.forward` is a smoothed quantity — and so is anything written from it. The same reasoning condemns the neighbouring `forward: bool` flag, which is a filtering quantity only on a history that was never converged. That is why the test at history.rs:1183 can assert the two evidences are equal. Left alone here; recorded as a follow-up. The design is a read-only forward-only pass instead: walk slices in time order carrying their own forward messages, and per slice build a scratch clone whose `backward` is `N_INF`, then run the unmodified production sweep on it. 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 today's evidence paths approximate it. Nothing is stored on `Skill`, which drops 16 bytes and helps #17 regardless. Three methods ship — `filtered_log_evidence`, `filtered_learning_curves`, `filtered_learning_curve` — all taking `&self`. The second consumer is ustat, whose learning curves start already collapsed to sigma 0.9-1.6 against a prior of 6.0 because every point is smoothed; the filtered view cannot be reconstructed from the public API today except by O(n^2) refits. The red test brackets the issue's own fixture strictly between 5*ln(0.5) and the batch evidence, so neither "still inert" nor "accidentally smoothed" passes. The invariant that would have caught this bug class is that filtered results are identical before and after `converge()` — exactly what a stored field cannot give. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01T5SYDExxL4vZgvunrcNSMc --- .../2026-08-27-filtered-estimates-design.md | 304 ++++++++++++++++++ 1 file changed, 304 insertions(+) create mode 100644 docs/superpowers/specs/2026-08-27-filtered-estimates-design.md diff --git a/docs/superpowers/specs/2026-08-27-filtered-estimates-design.md b/docs/superpowers/specs/2026-08-27-filtered-estimates-design.md new file mode 100644 index 0000000..14bcf16 --- /dev/null +++ b/docs/superpowers/specs/2026-08-27-filtered-estimates-design.md @@ -0,0 +1,304 @@ +# Filtered (Forward-Only) Estimates + +Closes [#19](https://git.aceofba.se/logaritmisk/trueskill-tt/issues/19). + +## Summary + +`HistoryBuilder::online(true)` is inert. It flips a flag that reaches +`Item::within_prior` (`src/time_slice.rs:70-71`), which reads +`Skill.online` (`src/time_slice.rs:25`) — a field initialised to `N_INF` +(`src/time_slice.rs:41`) and never assigned anywhere. The online path +therefore builds every rating from the improper Gaussian, and +`log_evidence()` silently reports `n × ln(0.5)`: every game scored as a +coin flip, finite and plausible-looking. + +This spec replaces the field and the flag with a **read-only forward-only +pass** over the converged history, exposed as three new public methods. +The pass reuses the production within-slice sweep verbatim rather than +reimplementing inference, and stores nothing on `Skill`. + +## Background + +### Why a stored field cannot hold this quantity + +The issue proposes populating `skill.online` during the forward pass, +alongside `new_forward_info` (`src/time_slice.rs:576`). That would not +work, and understanding why determines the whole design. + +`new_forward_info` sets `skill.forward` from +`agents[a].receive_for_elapsed(...)`, whose `message` was written by the +previous slice's `forward_prior_out` (`src/time_slice.rs:549`): + +```rust +skill.forward * skill.likelihood +``` + +`History::iteration` (`src/history.rs:255`) alternates a backward sweep +over slices and a forward sweep. From the second iteration onward, the +`skill.likelihood` feeding that message has already absorbed backward +information from the preceding backward sweep. So after `converge()`, +**`skill.forward` is a smoothed quantity, not a filtering one** — and any +field written from it inherits the same contamination on every sweep +after the first. + +### The neighbouring trap + +The same reasoning applies to the existing `forward: bool` parameter on +`log_evidence_internal` (`src/history.rs:395`). It is a genuine filtering +quantity only on a history that has never been converged. That is why the +test at `src/history.rs:1183` can assert + +```rust +assert_ulps_eq!(trueskill_log_evidence, trueskill_log_evidence_online, epsilon = 1e-6); +``` + +— the fixture is never converged, so the forward message still equals the +cavity prior. (Note also that the local binding is named `..._online` +while the flag it passes is `forward`; the two senses were already +muddled.) + +Fixing `forward: bool` is **out of scope** here; see *Out-of-scope +follow-ups*. + +### Why this is worth implementing rather than deleting + +The forward-only estimate has a second consumer beyond prequential model +comparison. `learning_curve()` returns post-convergence posteriors, so +every point is smoothed — the estimate at a given date incorporates +rounds played years later. On [ustat](https://git.aceofba.se/logaritmisk/ustat)'s +real data (prior μ=0, σ=6) that produces curves which start already +spread apart and barely move: + +``` +player first point final point +Eskil mu +3.72 sigma 1.17 mu +4.61 sigma 1.21 +Anders Olsson mu +1.61 sigma 0.90 mu +1.16 sigma 0.82 +LUDVIGSSON mu -2.09 sigma 1.08 mu -2.61 sigma 1.13 +Anners mu -2.85 sigma 1.27 mu -2.86 sigma 1.26 +``` + +σ at the *first* plotted point is 0.90–1.60 against a prior of 6.00. A +caller cannot reconstruct the filtered view from the public API today +except by refitting over `events[0..k]` for every k — O(n²) fits for +something one forward pass already computes. + +## Scope + +### What ships + +1. A read-only forward-only pass on `History`, walking slices in time + order and carrying its own forward messages. +2. Three public methods: `filtered_log_evidence`, + `filtered_learning_curves`, `filtered_learning_curve`. +3. Removal of `Skill.online`, `History.online`, `HistoryBuilder.online`, + `HistoryBuilder::online()`, and the `online: bool` parameter threaded + through `Item::within_prior`, `Event::within_priors`, and + `TimeSlice::log_evidence`. +4. `#[derive(Clone)]` on `Event`, `Team`, `Item`; `iterate_to_convergence` + loses its `#[cfg(test)]` gate. +5. A CHANGELOG entry recording the API break. + +### What does not ship + +- No change to `log_evidence()`, `log_evidence_for()`, `learning_curve()`, + `learning_curves()`, or `current_skill()`. Their values are unchanged + by this work. +- No fix to the `forward: bool` flag described above. +- No caching of pass results. Each call runs a full pass; the doc + comments say so. +- No `rayon` parallelism across slices — the pass is sequentially + dependent by construction. +- No prior-predictive accessor. The pass computes the pre-event forward + message internally, but only the filtered posterior is exposed until a + second caller needs otherwise. + +## Design + +### Naming + +`filtered_*`, not `online_*`. "Filtered" is the standard term for the +forward-only estimate, and the crate already uses "online" for a second, +unrelated thing — incremental ingestion, which `benches/baseline.txt:128` +calls the "online-add" path. Two senses of one word in one crate is how +the present bug reads as plausible. + +### The pass + +```rust +fn filtered_pass(&self) -> Vec<(T, Vec<(Index, Gaussian)>)> +``` + +Walk `self.time_slices` in order, carrying +`messages: HashMap` — the forward message out of each +competitor's most recent appearance. For each slice: + +1. **Build a scratch clone.** Same `time`, `p_draw`, `convergence`, and + cloned `events` with every `item.likelihood` reset to `N_INF`. Fresh + `SkillStore` in which, for each agent present in the real slice: + + ```rust + forward = match messages.get(&agent) { + Some(msg) => msg.forget(rating.drift.variance_for_elapsed(skill.elapsed)), + None => rating.prior, + } + backward = N_INF + likelihood = N_INF + elapsed = skill.elapsed // copied from the real slice + ``` + + This mirrors `Competitor::receive_for_elapsed` (`src/competitor.rs:38`) + 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 + convergence already trusts it. + +2. **Run the real sweep.** `scratch.iterate_to_convergence(agents)` + (`src/time_slice.rs:516`), unmodified. Fidelity comes from reusing the + production path rather than a parallel reimplementation — in + particular, a competitor appearing in two events at the same time is + handled by the same within-slice EP that `converge()` uses, not + approximated the way the current `online`/`forward` evidence paths are + (they run each event independently and sum). + +3. **Harvest.** With `backward == N_INF` acting as the multiplicative + identity, `Skill::posterior()` is exactly forward × likelihood — the + filtered posterior. Slice evidence is + `scratch.events.iter().map(|e| e.log_evidence).sum()`; `apply` + (`src/time_slice.rs:162`) writes that field on every event during the + sweep. + +4. **Carry forward.** `messages.insert(a, scratch.forward_prior_out(&a))` + for each agent in the slice. + +Steps 1–4 are the forward half of `History::iteration` +(`src/history.rs:283-297`) with the backward half never run. The pass +touches no field of `self`. + +### Public API + +```rust +impl, O: Observer, K: Eq + Hash + Clone> History { + pub fn filtered_log_evidence(&self) -> f64; + pub fn filtered_learning_curves(&self) -> HashMap>; + pub fn filtered_learning_curve(&self, key: &Q) -> Vec<(T, Gaussian)> + where + K: Borrow, + Q: Hash + Eq + ?Sized; +} +``` + +All take `&self` — the pass mutates nothing. Shapes deliberately mirror +`learning_curve` / `learning_curves` (`src/history.rs:325`, `:381`) so a +caller can plot smoothed and filtered curves on one chart with the same +handling code. + +`filtered_learning_curve` runs the same full pass as the plural form and +collects one key; the cost is identical, only the collection differs. +Callers wanting several keys should use the plural form. Documented on +both methods. + +Because the pass carries its own messages and re-runs inference, its +results **do not depend on whether `converge()` has been called**. That +is the property a stored field cannot have, and it is asserted as a test. + +### Removal inventory + +| Location | Change | +|---|---| +| `src/time_slice.rs:25` | delete `pub(crate) online: Gaussian` | +| `src/time_slice.rs:41` | delete `online: N_INF` from `Default` | +| `src/time_slice.rs:62,70-73` | drop `online` param and its branch from `Item::within_prior` | +| `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: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 | + +`Skill` loses 16 bytes, which is a small independent win for #17. + +## Testing strategy + +Every new test is mutation-proved before it counts: break the production +line it names, watch it fail for the *right* assertion, restore. A test +never observed failing is not evidence. + +### The red test + +On the issue's own fixture — five 1v1 games, same winner each time — +`filtered_log_evidence()` must land strictly between the two known +endpoints: + +``` +5 × ln(0.5) = -3.4657... (today's inert value) + < filtered + < -0.4012... (batch / smoothed evidence) +``` + +Two-sided, so neither "still inert" nor "accidentally smoothed" can pass. +The lower bound is right for a real reason: game one genuinely *is* a +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. +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`. +3. **Single-slice exactness** — for a history with one time slice there + is no future to propagate back, so filtered results equal smoothed + results exactly. +4. **Uncertainty ordering** — for a competitor with many later games, σ + at the first filtered point is greater than σ at the first smoothed + point, and less than the prior σ. This is the ustat complaint restated + as an assertion. +5. **Degenerate inputs** — empty history yields `0.0` and empty maps; + unknown key yields an empty curve. Added to + `tests/degenerate_inputs.rs`. + +### Regression net + +The existing suite must be unchanged by the removals: `log_evidence()`, +`log_evidence_for()`, and every numerical golden keep their current +values, since the default `online` was already `false` and the flag was +inert. + +## Verification gates + +- `just test` — full matrix, including the release job. `debug_assert!` + is compiled out in release, and that is where defects in this crate + have hidden before. +- `just lint` — clippy, warnings denied. +- `just fmt` — nightly. +- `just determinism` — the new pass must not perturb bit-identical + posteriors across `RAYON_NUM_THREADS` 1/2/4/8. +- `#![forbid(unsafe_code)]` stays. + +## Risks + +- **Clone cost.** One slice's events are cloned per slice visited. At + ustat scale this is negligible, but the pass is O(events) allocation on + top of O(events) inference. Accepted: fidelity to the production sweep + is worth more than avoiding the clone, and no caller is on a hot path. +- **`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. +- **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 + rather than via `..Default::default()`, so adding a field to `Skill` + is a compile error here rather than a silent wrong answer. + +## Out-of-scope follow-ups + +File as separate issues: + +1. **`forward: bool` is only a filtering quantity pre-convergence** + (`src/history.rs:395`). Either document the constraint or fold the + flag into the new pass and delete it. +2. **`log_evidence` takes `&mut self`** (`src/history.rs:416`) but + mutates nothing. The new `filtered_*` methods take `&self`; the + asymmetry is worth removing.