logaritmisk/trueskill-tt
1
0
Fork 0
Code Issues 1 Pull Requests Packages Projects Releases Activity

docs: spec for filtered (forward-only) estimates

Browse Source 
`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) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01T5SYDExxL4vZgvunrcNSMc
This commit is contained in:
logaritmisk
2026-08-27 15:42:54 +02:00
co-authored by Claude Opus 5
parent 9e8515b7cd
commit 4fde482e48
1 changed files with 304 additions and 0 deletions
Download Patch File Download Diff File Expand all files Collapse all files
docs/superpowers/specs/2026-08-27-filtered-estimates-design.md
+304
View File
@@ -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<Index, Gaussian>` — 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<T: Time, D: Drift<T>, O: Observer<T>, K: Eq + Hash + Clone> History<T, D, O, K> {
pub fn filtered_log_evidence(&self) -> f64;
pub fn filtered_learning_curves(&self) -> HashMap<K, Vec<(T, Gaussian)>>;
pub fn filtered_learning_curve<Q>(&self, key: &Q) -> Vec<(T, Gaussian)>
where
K: Borrow<Q>,
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.