HistoryBuilder::online(true) is non-functional — Skill.online is never written #19

New Issue

2026-08-04T19:19:10Z

logaritmisk commented

2026-08-04 19:19:10 +00:00

Skill.online (src/time_slice.rs:26) is read in exactly one place and written in none.

The only read is Item::within_prior (src/time_slice.rs:71-73):

if online {
    Rating::new(skill.online, r.beta, r.drift)
} else if forward { ... }

Skill::default() initialises it to N_INF (src/time_slice.rs:42) and nothing ever assigns to it — grep -rn "\.online" src/ returns that single read. So the online path always builds ratings from the improper/uninformative Gaussian.

Observable effect

self.online is threaded from the builder into TimeSlice::log_evidence (src/history.rs:357), so HistoryBuilder::online(true) changes what log_evidence() reports — to a value computed as though no competitor had any skill estimate at all:

five 1v1 games, same winner each time:
  online=false -> log_evidence = -0.4012074245943421
  online=true  -> log_evidence = -3.465736052799731

-3.4657… is exactly 5 × ln(0.5) — every game scored as a coin flip. The number is finite and plausible-looking, so nothing signals that the setting is inert.

What it was presumably meant to do

An "online" (filtering) evidence uses only information available before each event — the forward message, without the backward pass — which is the right quantity for prequential/predictive scoring and model comparison. That requires capturing the forward-only estimate into skill.online at the point where the forward sweep reaches each slice, before the backward info is folded in.

Fix

Either:

Implement it. Populate skill.online during the forward pass (alongside new_forward_info, src/time_slice.rs:512), and add a test asserting that online evidence differs from batch evidence in the expected direction and is not simply n × ln(0.5).
Remove it. Drop Skill.online, the online parameter threaded through within_prior/within_priors/log_evidence, and HistoryBuilder::online. That also shrinks Skill by 16 bytes, which matters given #17.

Shipping a builder method that silently changes a reported metric to a meaningless value is the worst of the three options.

Acceptance

Either online(true) produces a correct filtering log-evidence with a test that would fail against the current all-N_INF behaviour, or the setting is gone from the public API.
If removed, CHANGELOG.md records the API break.

`Skill.online` (`src/time_slice.rs:26`) is read in exactly one place and written in none. The only read is `Item::within_prior` (`src/time_slice.rs:71-73`): ```rust if online { Rating::new(skill.online, r.beta, r.drift) } else if forward { ... } ``` `Skill::default()` initialises it to `N_INF` (`src/time_slice.rs:42`) and nothing ever assigns to it — `grep -rn "\.online" src/` returns that single read. So the online path always builds ratings from the improper/uninformative Gaussian. ## Observable effect `self.online` is threaded from the builder into `TimeSlice::log_evidence` (`src/history.rs:357`), so `HistoryBuilder::online(true)` changes what `log_evidence()` reports — to a value computed as though no competitor had any skill estimate at all: ``` five 1v1 games, same winner each time: online=false -> log_evidence = -0.4012074245943421 online=true -> log_evidence = -3.465736052799731 ``` `-3.4657…` is exactly `5 × ln(0.5)` — every game scored as a coin flip. The number is finite and plausible-looking, so nothing signals that the setting is inert. ## What it was presumably meant to do An "online" (filtering) evidence uses only information available *before* each event — the forward message, without the backward pass — which is the right quantity for prequential/predictive scoring and model comparison. That requires capturing the forward-only estimate into `skill.online` at the point where the forward sweep reaches each slice, before the backward info is folded in. ## Fix Either: - **Implement it.** Populate `skill.online` during the forward pass (alongside `new_forward_info`, `src/time_slice.rs:512`), and add a test asserting that online evidence differs from batch evidence in the expected direction and is *not* simply `n × ln(0.5)`. - **Remove it.** Drop `Skill.online`, the `online` parameter threaded through `within_prior`/`within_priors`/`log_evidence`, and `HistoryBuilder::online`. That also shrinks `Skill` by 16 bytes, which matters given #17. Shipping a builder method that silently changes a reported metric to a meaningless value is the worst of the three options. ## Acceptance - Either `online(true)` produces a correct filtering log-evidence with a test that would fail against the current all-`N_INF` behaviour, or the setting is gone from the public API. - If removed, `CHANGELOG.md` records the API break.

logaritmisk referenced this issue

2026-08-04 19:20:44 +00:00

Dead and duplicated code: a stale `iterate_to_convergence`, a copy-pasted sweep, and `#[allow(dead_code)]` used as a mute button #23

logaritmisk referenced this issue

2026-08-04 19:21:02 +00:00

No CI — nothing verifies builds, tests, feature combinations, or determinism #24

logaritmisk referenced this issue

2026-08-04 19:21:40 +00:00

Test suite has no coverage for degenerate inputs, release-mode behaviour, or non-finite results #26

logaritmisk commented

2026-08-27 13:29:18 +00:00

A second consumer for the same machinery turned up in ustat, which bears on the implement-vs-remove choice above.

The use case: a filtered learning curve, not just filtered evidence.

learning_curve() returns post-convergence posteriors, so every point is smoothed — the estimate at a given date incorporates rounds played years later. That is usually what you want, but it produces a chart that surprises people: six players' curves start already spread apart and barely move, because the model has propagated all its later confidence backwards.

Measured on ustat's real data (prior is μ=0, σ=6):

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 — the smoother has already collapsed the uncertainty before the first round is drawn. Anners' μ moves 0.01 across his entire history.

The natural fix on the app side is a "what we knew at the time" view: the forward-only estimate, where everyone genuinely starts at the prior and fans out. That is the same forward-message quantity this issue is about, just exposed as a posterior per time slice rather than folded into an evidence number.

Why this argues for implement over remove. Removal is clean if online only ever serves prequential model comparison. But the forward-only estimate has a second, user-facing use, and it cannot be reconstructed from the public API today — a caller can only refit incrementally over events[0..k] for every k, which is O(n²) fits for something the forward sweep already computes once.

A suggestion for the shape, if implemented. Populating skill.online during the forward pass would fix the evidence path as described. It would be worth also exposing the filtered value, e.g. a filtered_learning_curve(key) alongside learning_curve(key), so the forward estimate is reachable without going through log_evidence. That makes the field's cost buy two features rather than one, which changes the trade against #17.

No urgency from ustat's side — it has been noted there as a possible feature, not a blocker.

A second consumer for the same machinery turned up in [ustat](https://git.aceofba.se/logaritmisk/ustat), which bears on the implement-vs-remove choice above. **The use case: a filtered learning curve, not just filtered evidence.** `learning_curve()` returns post-convergence posteriors, so every point is smoothed — the estimate at a given date incorporates rounds played years later. That is usually what you want, but it produces a chart that surprises people: six players' curves start already spread apart and barely move, because the model has propagated all its later confidence backwards. Measured on ustat's real data (prior is μ=0, σ=6): ``` 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 — the smoother has already collapsed the uncertainty before the first round is drawn. Anners' μ moves 0.01 across his entire history. The natural fix on the app side is a "what we knew at the time" view: the forward-only estimate, where everyone genuinely starts at the prior and fans out. That is the same forward-message quantity this issue is about, just exposed as a *posterior per time slice* rather than folded into an evidence number. **Why this argues for implement over remove.** Removal is clean if `online` only ever serves prequential model comparison. But the forward-only estimate has a second, user-facing use, and it cannot be reconstructed from the public API today — a caller can only refit incrementally over `events[0..k]` for every k, which is O(n²) fits for something the forward sweep already computes once. **A suggestion for the shape, if implemented.** Populating `skill.online` during the forward pass would fix the evidence path as described. It would be worth also exposing the filtered value, e.g. a `filtered_learning_curve(key)` alongside `learning_curve(key)`, so the forward estimate is reachable without going through `log_evidence`. That makes the field's cost buy two features rather than one, which changes the trade against #17. No urgency from ustat's side — it has been noted there as a possible feature, not a blocker.

logaritmisk referenced this issue

2026-08-27 14:48:53 +00:00

log_evidence_internal's `forward` flag is only a filtering quantity before convergence #28

logaritmisk referenced this issue

2026-08-27 14:48:55 +00:00

log_evidence takes &mut self but mutates nothing #29

logaritmisk referenced this issue

2026-08-27 14:48:59 +00:00

Spec's tolerance-not-bit-identity caveat for filtered estimates is conservative #30

logaritmisk referenced this issue

2026-08-27 14:59:47 +00:00

Skill's compile-time divergence tripwire is only half-enforced #31

logaritmisk commented

2026-08-27 15:12:48 +00:00

Fixed on main (bf9d964..69ddebe). Design doc: docs/superpowers/specs/2026-08-27-filtered-estimates-design.md.

Both acceptance criteria are met — the setting is gone from the public API and a correct filtering log-evidence ships.

The issue's proposed fix would not have worked, and that reshaped the design. Populating skill.online during the forward pass inherits the contamination it was meant to avoid: new_forward_info sets skill.forward from the previous slice's forward_prior_out = forward * likelihood, and 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. A stored field cannot hold a filtering estimate here.

What shipped instead is a read-only forward-only pass: it walks slices in time order carrying its own forward messages, and per slice clones the slice into a scratch whose backward is left at N_INF, then runs the unmodified production sweep on that scratch. 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 — which drops 16 bytes, incidentally helping #17.

Public API: filtered_log_evidence(), filtered_learning_curves(), filtered_learning_curve(key) — all &self. HistoryBuilder::online, History.online, Skill.online and the threaded online: bool parameter are deleted.

The test that would have failed against the old behaviour, on this issue's own fixture (five 1v1 games, same winner):

coin flip  -3.4657359027997265   <- what online(true) reported: 5 x ln(0.5)
filtered   -1.0670128892359618   <- new
batch      -0.4012074245943422   <- smoothed, matches this issue's quoted value to 1 ulp

Bracketed strictly between both bounds, so neither "still inert" nor "accidentally smoothed" can pass. Game one genuinely is a coin flip under filtering; games two through five are not.

The invariant that pins the bug class: filtered results must be identical whether or not converge() has run. Mutating the implementation to read skill.forward instead of the carried message moves the evidence from -1.0926192830182704 to -1.6709488207437966 — exactly the drift a stored field would have shipped.

Also addressed the second use case from the comment above. filtered_learning_curve gives the "what we knew at the time" view. On a 12-game fixture, first-point sigma is 4.985 filtered vs 3.386 smoothed against a prior of 6.0 — the smoother had already collapsed uncertainty before the first round was drawn, which is the charting complaint. It is one O(events) pass rather than the O(n^2) refits the public API previously forced.

Follow-ups filed: #28 (the forward flag is only a filtering quantity pre-convergence — the same trap, one function over), #29, #30, #31.

Fixed on `main` (`bf9d964`..`69ddebe`). Design doc: `docs/superpowers/specs/2026-08-27-filtered-estimates-design.md`. **Both acceptance criteria are met** — the setting is gone from the public API *and* a correct filtering log-evidence ships. **The issue's proposed fix would not have worked, and that reshaped the design.** Populating `skill.online` during the forward pass inherits the contamination it was meant to avoid: `new_forward_info` sets `skill.forward` from the previous slice's `forward_prior_out` = `forward * likelihood`, and `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. **A stored field cannot hold a filtering estimate here.** What shipped instead is a read-only forward-only pass: it walks slices in time order carrying its own forward messages, and per slice clones the slice into a scratch whose `backward` is left at `N_INF`, then runs the *unmodified* production sweep on that scratch. 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` — which drops 16 bytes, incidentally helping #17. **Public API:** `filtered_log_evidence()`, `filtered_learning_curves()`, `filtered_learning_curve(key)` — all `&self`. `HistoryBuilder::online`, `History.online`, `Skill.online` and the threaded `online: bool` parameter are deleted. **The test that would have failed against the old behaviour**, on this issue's own fixture (five 1v1 games, same winner): ``` coin flip -3.4657359027997265 <- what online(true) reported: 5 x ln(0.5) filtered -1.0670128892359618 <- new batch -0.4012074245943422 <- smoothed, matches this issue's quoted value to 1 ulp ``` Bracketed strictly between both bounds, so neither "still inert" nor "accidentally smoothed" can pass. Game one genuinely *is* a coin flip under filtering; games two through five are not. **The invariant that pins the bug class:** filtered results must be identical whether or not `converge()` has run. Mutating the implementation to read `skill.forward` instead of the carried message moves the evidence from `-1.0926192830182704` to `-1.6709488207437966` — exactly the drift a stored field would have shipped. **Also addressed the second use case from the comment above.** `filtered_learning_curve` gives the "what we knew at the time" view. On a 12-game fixture, first-point sigma is 4.985 filtered vs 3.386 smoothed against a prior of 6.0 — the smoother had already collapsed uncertainty before the first round was drawn, which is the charting complaint. It is one O(events) pass rather than the O(n^2) refits the public API previously forced. Follow-ups filed: #28 (the `forward` flag is only a filtering quantity pre-convergence — the same trap, one function over), #29, #30, #31.

logaritmisk closed this issue

2026-08-27 15:12:51 +00:00

logaritmisk referenced this issue from a commit

2026-08-27 15:37:44 +00:00

docs: spec for filtered (forward-only) estimates

logaritmisk referenced this issue from a commit

2026-08-27 15:37:44 +00:00

test: pin the invariants that make filtered estimates trustworthy

logaritmisk referenced this issue from a commit

2026-08-27 15:37:44 +00:00

fix: close out four small issues and pin #27's repro

logaritmisk referenced this issue