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

predict_* returns Ok on a NaN-poisoned fit, and eight # Errors sections do not match the code #78

New Issue
Closed
opened 2026-09-09 17:57:41 +00:00 by logaritmisk · 2 comments
logaritmisk commented 2026-09-09 17:57:41 +00:00
Owner
Copy Link
Copy Source

1. Prediction paths have no non-finite guard

converge gained a NonFiniteResult guard; no prediction path has one. Measured on a history seeded with point-mass priors and beta(0.0):

current_skill(a).sigma()   = NaN
predict_quality            = Ok(NaN)
predict_win_probabilities  = Ok([0.0, 0.0])   <- finite, sums to 0, doc says "sum to one"
predict_outcome().total()  = NaN

Ok([0.0, 0.0]) is the dangerous one: finite and plausible, which is precisely the shape GridTooCoarse was introduced to stop. A caller checking total() ≈ 1 catches the third row and not the second.

Related: quality(&[&[σ=0], &[σ=0]], 1.0) returns NaN, which the # Panics doc does not mention — it describes only the beta == 0 panic.

Fix: a shared guard where skills are read (performances / member_skills), returning NonFiniteResult. Note its step: (f64, f64) field name is convergence vocabulary and reads oddly outside it — worth values: or a distinct variant (see #74).

Breaks: callers currently receiving Ok(NaN) now receive Err. That is the point.

2. Eight documented # Errors sections are wrong

Function Problem
History::expected_information_gain # Errors section is empty — # Preconditions follows immediately and the error list is stranded inside it. Renders as a blank Errors block.
History::predict_ranking Same empty-section bug
History::predict_ranking Omits GridTooCoarse — measured
History::predict_outcome Omits GridTooCoarse — measured
expected_information_gain (free) Omits GridTooCoarse; it comes from predict::outcome_distribution, not Game::ranked, so "anything Game::ranked returns" does not cover it
History::predict_margin Says "JointUnavailable if the latest slice holds ranked events". Measured with an early ranked slice and a late scored one: it fails. The condition is all slices. Also omits the empty-history case.
History::add_events Documents three errors; measured five more: NotEnoughTeams, EmptyTeam, ConflictingCompetitorConfig, InvalidParameter{drift_scale}, InvalidParameter{score/rank/weight}
History::add_events Claims MismatchedShape "if per-member weights do not match the team's membership" — unreachable through add_events; weights come 1:1 off Member.weight. That check lives only in EventBuilder::weights.
converge / converge_partial Both omit InvalidParameter { name: "drift variance" }

The two empty-# Errors sections are the priority — they are silently blank in published docs.

3. Five methods silently require an all-scored history

joint, posterior_of, posterior_of_at, expected_variance_reduction and predict_margin all fail on any history containing a ranked event — which is the history the getting-started path teaches you to build, since record_winner produces ranked events. A user follows the quickstart, sees posterior_of in the method list, and it never works. The restriction is discoverable only in joint()'s # Errors, three methods away.

This is a symptom of a layering problem worth deciding on its own. Joint is a well-designed handle — it holds the factorisation, the borrow enforces what a cache would have to invalidate by hand, and its docs explain the O(n³)/O(n²) split. But all three of its methods are also mirrored onto History as one-shot wrappers, and that is how the scored-only restriction got smuggled onto the flat surface with no signal.

Consider deleting the mirrors. h.joint()?.posterior_of(..) is one call longer and tells the truth: you need a joint, and a joint needs a scored history. That leaves a clean three-tier shape — History for fit-and-read, a predict() handle for the six prediction methods that share preconditions, and Joint for exact joint work.

4. predict_ is the wrong prefix on quality

The genuine predict_* family returns a probability or distribution over what will happen. predict_quality predicts nothing — its own doc says it answers "is this matchup fair". And the free/method pair is inconsistent: free quality ↔ predict_quality, but free expected_information_gain ↔ expected_information_gain (same name).

The rule the code already follows is sound and just needs stating: a free function scores a hypothetical from explicit parameters; a History method asks the same question against the fit. Only the naming fails to express it. History::quality (no prefix) makes the pair consistent — a method sharing a name with a free function is normal Rust.

expected_variance_reduction and posterior_of are correctly outside predict_*: the former's own docs say "there is no expectation to take", and the latter reads the fit rather than forecasting.

Breaks: predict_quality callers; the History mirrors if removed.

Found by an API audit, 2026-09-09.

## 1. Prediction paths have no non-finite guard `converge` gained a `NonFiniteResult` guard; **no prediction path has one.** Measured on a history seeded with point-mass priors and `beta(0.0)`: ``` current_skill(a).sigma() = NaN predict_quality = Ok(NaN) predict_win_probabilities = Ok([0.0, 0.0]) <- finite, sums to 0, doc says "sum to one" predict_outcome().total() = NaN ``` `Ok([0.0, 0.0])` is the dangerous one: finite and plausible, which is precisely the shape `GridTooCoarse` was introduced to stop. A caller checking `total() ≈ 1` catches the third row and not the second. Related: `quality(&[&[σ=0], &[σ=0]], 1.0)` returns `NaN`, which the `# Panics` doc does not mention — it describes only the `beta == 0` panic. **Fix:** a shared guard where skills are read (`performances` / `member_skills`), returning `NonFiniteResult`. Note its `step: (f64, f64)` field name is convergence vocabulary and reads oddly outside it — worth `values:` or a distinct variant (see #74). **Breaks:** callers currently receiving `Ok(NaN)` now receive `Err`. That is the point. ## 2. Eight documented `# Errors` sections are wrong | Function | Problem | |---|---| | `History::expected_information_gain` | **`# Errors` section is empty** — `# Preconditions` follows immediately and the error list is stranded inside it. Renders as a blank Errors block. | | `History::predict_ranking` | Same empty-section bug | | `History::predict_ranking` | Omits `GridTooCoarse` — measured | | `History::predict_outcome` | Omits `GridTooCoarse` — measured | | `expected_information_gain` (free) | Omits `GridTooCoarse`; it comes from `predict::outcome_distribution`, not `Game::ranked`, so "anything `Game::ranked` returns" does not cover it | | `History::predict_margin` | Says *"`JointUnavailable` if the **latest slice** holds ranked events"*. Measured with an early ranked slice and a late scored one: it fails. The condition is **all** slices. Also omits the empty-history case. | | `History::add_events` | Documents three errors; measured **five more**: `NotEnoughTeams`, `EmptyTeam`, `ConflictingCompetitorConfig`, `InvalidParameter{drift_scale}`, `InvalidParameter{score/rank/weight}` | | `History::add_events` | Claims `MismatchedShape` *"if per-member weights do not match the team's membership"* — **unreachable** through `add_events`; weights come 1:1 off `Member.weight`. That check lives only in `EventBuilder::weights`. | | `converge` / `converge_partial` | Both omit `InvalidParameter { name: "drift variance" }` | The two empty-`# Errors` sections are the priority — they are silently blank in published docs. ## 3. Five methods silently require an all-scored history `joint`, `posterior_of`, `posterior_of_at`, `expected_variance_reduction` and `predict_margin` all fail on any history containing a ranked event — which is the history the getting-started path teaches you to build, since `record_winner` produces ranked events. A user follows the quickstart, sees `posterior_of` in the method list, and it never works. The restriction is discoverable only in `joint()`'s `# Errors`, three methods away. This is a symptom of a layering problem worth deciding on its own. `Joint` is a well-designed handle — it holds the factorisation, the borrow enforces what a cache would have to invalidate by hand, and its docs explain the O(n³)/O(n²) split. But all three of its methods are **also mirrored onto `History`** as one-shot wrappers, and that is how the scored-only restriction got smuggled onto the flat surface with no signal. **Consider deleting the mirrors.** `h.joint()?.posterior_of(..)` is one call longer and tells the truth: you need a joint, and a joint needs a scored history. That leaves a clean three-tier shape — `History` for fit-and-read, a `predict()` handle for the six prediction methods that share preconditions, and `Joint` for exact joint work. ## 4. `predict_` is the wrong prefix on `quality` The genuine `predict_*` family returns a probability or distribution over what will happen. `predict_quality` predicts nothing — its own doc says it answers *"is this matchup **fair**"*. And the free/method pair is inconsistent: free `quality` ↔ `predict_quality`, but free `expected_information_gain` ↔ `expected_information_gain` (same name). The rule the code already follows is sound and just needs stating: a **free function** scores a hypothetical from explicit parameters; a **`History` method** asks the same question against the fit. Only the naming fails to express it. `History::quality` (no prefix) makes the pair consistent — a method sharing a name with a free function is normal Rust. `expected_variance_reduction` and `posterior_of` are correctly outside `predict_*`: the former's own docs say *"there is no expectation to take"*, and the latter reads the fit rather than forecasting. **Breaks:** `predict_quality` callers; the `History` mirrors if removed. Found by an API audit, 2026-09-09.
logaritmisk added the apibugdocs labels 2026-09-09 17:58:33 +00:00
logaritmisk commented 2026-09-09 19:41:05 +00:00
Author
Owner
Copy Link
Copy Source

Parts 1 and 2 are done in 9d3e002 (merged as 78810c0). Parts 3 and 4 are design calls and stay open — see the bottom.

Part 1 — reproduced, then fixed

The three rows reproduce exactly as reported. Both checks now live at member_skills, the one gate every prediction path reads skills through, rather than being repeated per method.

A second, worse case turned up next to it. The same parameters on a scored event converge cleanly and leave legitimate point-mass posteriors (pi: inf, mu: 0, sigma: 0 — a correct fit, not a broken one). On that history:

converge          = Ok(ConvergenceReport { converged: true, .. })
predict_quality   = panicked at src/matrix.rs:221: cannot invert a singular matrix
predict_win_probs = Ok([0.0, 0.0])

A Result-returning method panicking, from a history that converged. The free quality documents this panic under # Panics, so it behaves as specified; predict_quality is the one that must not. The contrast covariance beta²AᵀA + AᵀSA is exactly singular when beta is zero and every skill is a point mass, and predict_win_probabilities returns zeros for the same underlying reason: the "sum to one at p_draw == 0" promise assumes continuous performances, where an exact tie has measure zero. Point masses break that assumption, not the arithmetic. Both now return InvalidParameter.

One correction to my own first attempt. I checked pi and tau for finiteness — the natural parameters, on the reasoning that mu() and sigma() guard pi <= 0 and so report accessor policy rather than message state. Measurement said otherwise: a legitimate point mass is pi = inf, and that check rejected it, turning a working prediction into an error. The guard is on mu / sigma, which are what predictions actually consume.

tests/prediction_guards.rs covers both fixtures across all five prediction methods, with a healthy control that must still answer everything and must still sum its win probabilities to one — so the guards cannot be satisfied by making every path fail.

Not reproduced: quality(&[&[σ=0], &[σ=0]], 1.0) returns 1, not NaN. The ln_abs_determinant rewrite fixed that, and 1 is the right answer — two identical point masses with beta = 1 are a perfectly fair matchup.

Part 2 — most were already fixed; two were real

Re-checked all eight against the current tree. The empty # Errors sections, the missing GridTooCoarse on predict_ranking/predict_outcome, the predict_margin "latest slice" wording, and the add_events list (including the note that MismatchedShape on weights belongs to EventBuilder::weights) had all been fixed by the earlier api/cleanup work. The table was accurate when written and had gone stale.

Two survived and are fixed here:

  • converge_partial omitted the drift-variance InvalidParameter it validates before sweeping.
  • Free expected_information_gain omitted GridTooCoarse. It comes from predict::outcome_distribution, which runs before any inference, so the existing "anything Game::ranked returns" clause genuinely did not cover it — as the issue said.

All six prediction methods now document the two errors the shared gate adds.

Still open

Part 3 (layering) and part 4 (predict_quality → quality) both change the shape of the public API rather than fixing a wrong answer. Part 3 in particular argues for deleting the History mirrors of Joint's methods, which is a call about what the crate's front door should look like. Neither is something to decide unilaterally.

NonFiniteResult's step: (f64, f64) field name still reads oddly outside convergence — it now carries (mu, sigma) on the prediction path. Left to #74, which owns the error vocabulary.

**Parts 1 and 2 are done** in 9d3e002 (merged as 78810c0). Parts 3 and 4 are design calls and stay open — see the bottom. ## Part 1 — reproduced, then fixed The three rows reproduce exactly as reported. Both checks now live at `member_skills`, the one gate every prediction path reads skills through, rather than being repeated per method. **A second, worse case turned up next to it.** The same parameters on a *scored* event converge cleanly and leave legitimate point-mass posteriors (`pi: inf`, `mu: 0`, `sigma: 0` — a correct fit, not a broken one). On that history: ``` converge = Ok(ConvergenceReport { converged: true, .. }) predict_quality = panicked at src/matrix.rs:221: cannot invert a singular matrix predict_win_probs = Ok([0.0, 0.0]) ``` A `Result`-returning method panicking, from a history that converged. The free `quality` documents this panic under `# Panics`, so it behaves as specified; `predict_quality` is the one that must not. The contrast covariance `beta²AᵀA + AᵀSA` is exactly singular when beta is zero and every skill is a point mass, and `predict_win_probabilities` returns zeros for the same underlying reason: the "sum to one at `p_draw == 0`" promise assumes continuous performances, where an exact tie has measure zero. Point masses break that assumption, not the arithmetic. Both now return `InvalidParameter`. **One correction to my own first attempt.** I checked `pi` and `tau` for finiteness — the natural parameters, on the reasoning that `mu()` and `sigma()` guard `pi <= 0` and so report accessor policy rather than message state. Measurement said otherwise: a legitimate point mass is `pi = inf`, and that check rejected it, turning a working prediction into an error. The guard is on `mu` / `sigma`, which are what predictions actually consume. `tests/prediction_guards.rs` covers both fixtures across all five prediction methods, with a healthy control that must still answer everything and must still sum its win probabilities to one — so the guards cannot be satisfied by making every path fail. Not reproduced: `quality(&[&[σ=0], &[σ=0]], 1.0)` returns `1`, not `NaN`. The `ln_abs_determinant` rewrite fixed that, and `1` is the right answer — two identical point masses with `beta = 1` are a perfectly fair matchup. ## Part 2 — most were already fixed; two were real Re-checked all eight against the current tree. The empty `# Errors` sections, the missing `GridTooCoarse` on `predict_ranking`/`predict_outcome`, the `predict_margin` "latest slice" wording, and the `add_events` list (including the note that `MismatchedShape` on weights belongs to `EventBuilder::weights`) had all been fixed by the earlier `api/cleanup` work. The table was accurate when written and had gone stale. Two survived and are fixed here: - `converge_partial` omitted the drift-variance `InvalidParameter` it validates before sweeping. - Free `expected_information_gain` omitted `GridTooCoarse`. It comes from `predict::outcome_distribution`, which runs before any inference, so the existing "anything `Game::ranked` returns" clause genuinely did not cover it — as the issue said. All six prediction methods now document the two errors the shared gate adds. ## Still open **Part 3 (layering)** and **part 4 (`predict_quality` → `quality`)** both change the shape of the public API rather than fixing a wrong answer. Part 3 in particular argues for deleting the `History` mirrors of `Joint`'s methods, which is a call about what the crate's front door should look like. Neither is something to decide unilaterally. `NonFiniteResult`'s `step: (f64, f64)` field name still reads oddly outside convergence — it now carries `(mu, sigma)` on the prediction path. Left to #74, which owns the error vocabulary.
logaritmisk commented 2026-09-10 04:40:47 +00:00
Author
Owner
Copy Link
Copy Source

Parts 3 and 4 are done, so this closes. e72bf38 (merged as d2ab444) for the layering; 13a395f for the rename.

Part 3 — mirrors deleted

h.joint()?.posterior_of(..) is the only path now. The three-tier shape you described: History fits and reads, predict_* forecasts, Joint answers exact joint questions.

predict_margin was itself calling self.posterior_of, so it goes through self.joint()? directly.

One correction to the issue. The complaint that the restriction is "discoverable only in joint()'s # Errors, three methods away" had already gone stale — the api/cleanup docs pass gave posterior_of its own # Limitations and # Errors sections covering it. So the deletion is not buying discoverability; it is buying the honest cost model. The wrappers re-factorised on every call, warning the reader in their own docs to take a Joint instead, which is a strange thing for a method to say about itself.

Going the other way turned up something: the Joint methods' docs deferred to the wrappers for their real content ("Identical to History::posterior_of, without re-paying the factorisation"). Deleting the wrappers would have left the surviving API documented by reference to something that no longer existed, so the substance moved onto them — what a linear functional means, which appearance each competitor is read at, and why expected_variance_reduction belongs on the handle rather than beside it.

tests/joint_handle.rs had three tests whose entire purpose was comparing wrapper against handle. Rather than delete them, they now compare a reused joint against a fresh one per question — which is the actual correctness claim behind caching the factorisation (#51), with the wrapper taken out of the middle.

Part 4 — History::quality

Renamed. The free/method pair is consistent now: free quality ↔ History::quality, matching free expected_information_gain ↔ History::expected_information_gain. The rule that was already being followed and never written down — a free function scores a hypothetical from explicit parameters, the same-named method asks it against the fit — is now stated on the method.

NonFiniteResult's step field name is still overloaded (sweep step from converge, (mu, sigma) from a prediction). Left to #74, which owns the error vocabulary.

Parts 3 and 4 are done, so this closes. e72bf38 (merged as d2ab444) for the layering; 13a395f for the rename. ## Part 3 — mirrors deleted `h.joint()?.posterior_of(..)` is the only path now. The three-tier shape you described: `History` fits and reads, `predict_*` forecasts, `Joint` answers exact joint questions. `predict_margin` was itself calling `self.posterior_of`, so it goes through `self.joint()?` directly. **One correction to the issue.** The complaint that the restriction is "discoverable only in `joint()`'s `# Errors`, three methods away" had already gone stale — the `api/cleanup` docs pass gave `posterior_of` its own `# Limitations` and `# Errors` sections covering it. So the deletion is not buying discoverability; it is buying the honest cost model. The wrappers re-factorised on every call, warning the reader in their own docs to take a `Joint` instead, which is a strange thing for a method to say about itself. Going the other way turned up something: the `Joint` methods' docs *deferred to the wrappers* for their real content ("Identical to `History::posterior_of`, without re-paying the factorisation"). Deleting the wrappers would have left the surviving API documented by reference to something that no longer existed, so the substance moved onto them — what a linear functional means, which appearance each competitor is read at, and why `expected_variance_reduction` belongs on the handle rather than beside it. `tests/joint_handle.rs` had three tests whose entire purpose was comparing wrapper against handle. Rather than delete them, they now compare a *reused* joint against a *fresh* one per question — which is the actual correctness claim behind caching the factorisation (#51), with the wrapper taken out of the middle. ## Part 4 — `History::quality` Renamed. The free/method pair is consistent now: free `quality` ↔ `History::quality`, matching free `expected_information_gain` ↔ `History::expected_information_gain`. The rule that was already being followed and never written down — a free function scores a hypothetical from explicit parameters, the same-named method asks it against the fit — is now stated on the method. `NonFiniteResult`'s `step` field name is still overloaded (sweep step from `converge`, `(mu, sigma)` from a prediction). Left to #74, which owns the error vocabulary.
Sign in to join this conversation.
No Branch/Tag Specified
Branches Tags
Labels
Clear labels
api
Shape or ergonomics of the public surface
 blocked
Waiting on another issue or an external decision
 breaking
Resolving it changes the public API; wants a major bump
 bug
Behaviour is wrong, including silently wrong
 decision
Needs a call to be made, not code to be written
 docs
Documentation, including doc comments and the README
 enhancement
New capability the crate does not have
 infra
CI, release process, packaging
 numerics
Floating-point accuracy, cancellation, convergence maths
 performance
Speed or memory
 refactor
Internal cleanup with no intended behaviour change
 testing
Test coverage and test infrastructure
No labels api bug docs
Milestone
No items
No Milestone
Projects
Clear projects
No projects
Assignees
Clear assignees
logaritmisk
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: logaritmisk/trueskill-tt#78