UnknownKey reports indices, not the key — and rejecting the whole matchup is easy to swallow into a plausible constant #44

New Issue

2026-09-07T21:20:27Z

logaritmisk commented

2026-09-07 21:20:27 +00:00

What happened downstream

Upgrading lester-stash from 0.1.2 to 0.4.1 silently disabled its entire metadata model for a day. Every call to predict_outcome returned Err(UnknownKey { .. }) — 5591 out of 5591 — and the caller's error branch returned a neutral 0.5.

Nothing crashed. Nothing logged. Every prediction became a coin flip, and the seeding signal vanished. It was found only by sweeping an unrelated parameter and noticing the output did not move: implied = mu_r + scale * (p_r - 0.5) is scale-invariant only if p_r is exactly 0.5.

The behaviour change itself is correct and welcome — 0.1.2 dropped unknown keys silently inside predict_outcome, which was worse. Two things about its ergonomics made the failure hard to see.

1. The error does not say which key

// src/error.rs:53
UnknownKey { team: usize, member: usize },

UnknownKey { team: 0, member: 0 } tells a caller nothing actionable. The key is the one thing needed to diagnose it, and History is already generic over K. Even K: Debug on the error path, or carrying the key as a String via format!("{key:?}") at construction, would turn an opaque failure into a self-explaining one.

In our case the unknown keys were perfectly ordinary — tags and performers that had simply never appeared in a training event. The error being anonymous is what let it read as "something is malformed" rather than "you are passing keys I have not seen, which for you is routine".

2. All-or-nothing rejection is a sharp edge for the common case

A caller with a partially-known team has three options today:

Pre-filter with current_skill(k).is_some() before calling — what we now do. Correct, but every key is resolved twice: once by the filter, once inside predict_outcome.
Let it error and fall back — what we did, and it silently destroyed the signal.
Track known keys separately, duplicating state the History already holds.

An opt-in mode would remove the sharp edge without weakening the default:

pub enum UnknownKeys {
    /// Reject the matchup. Today's behaviour, and the right default.
    Reject,
    /// Ignore unknown members; a team with no known members is still an error.
    Skip,
}

Even without that, one line in predict_outcome's docs — "every key must already be known to the history; pre-filter with lookup/current_skill if that is not guaranteed" — would have saved the day. The doc currently describes what the function returns, not what it requires.

Why this is worth more than the sum of its parts

This crate's recurring defect, in its own issue history, is a public surface that reports a plausible constant. This is the caller-side twin: an error so opaque that the natural handling turns it into one. A neutral 0.5 from a prediction function is indistinguishable from a real answer, so the failure is invisible to every test that does not assert on variation.

Our regression test now pins it, and it is worth stating what it took to find: no exception, no log line, no failing test — only a parameter sweep that produced suspiciously identical numbers.

Concrete asks, smallest first

Include the key in UnknownKey (or its Display).
Document the precondition on predict_outcome, predict_quality and expected_information_gain.
Consider an opt-in UnknownKeys::Skip, which would also remove the double resolution.

## What happened downstream Upgrading lester-stash from 0.1.2 to 0.4.1 silently disabled its entire metadata model for a day. Every call to `predict_outcome` returned `Err(UnknownKey { .. })` — **5591 out of 5591** — and the caller's error branch returned a neutral `0.5`. Nothing crashed. Nothing logged. Every prediction became a coin flip, and the seeding signal vanished. It was found only by sweeping an unrelated parameter and noticing the output did not move: `implied = mu_r + scale * (p_r - 0.5)` is scale-invariant only if `p_r` is exactly 0.5. The behaviour change itself is **correct and welcome** — 0.1.2 dropped unknown keys silently inside `predict_outcome`, which was worse. Two things about its ergonomics made the failure hard to see. ## 1. The error does not say which key ```rust // src/error.rs:53 UnknownKey { team: usize, member: usize }, ``` `UnknownKey { team: 0, member: 0 }` tells a caller nothing actionable. The key is the one thing needed to diagnose it, and `History` is already generic over `K`. Even `K: Debug` on the error path, or carrying the key as a `String` via `format!("{key:?}")` at construction, would turn an opaque failure into a self-explaining one. In our case the unknown keys were perfectly ordinary — tags and performers that had simply never appeared in a training event. The error being anonymous is what let it read as "something is malformed" rather than "you are passing keys I have not seen, which for you is routine". ## 2. All-or-nothing rejection is a sharp edge for the common case A caller with a partially-known team has three options today: 1. Pre-filter with `current_skill(k).is_some()` before calling — what we now do. Correct, but every key is resolved **twice**: once by the filter, once inside `predict_outcome`. 2. Let it error and fall back — what we did, and it silently destroyed the signal. 3. Track known keys separately, duplicating state the `History` already holds. An opt-in mode would remove the sharp edge without weakening the default: ```rust pub enum UnknownKeys { /// Reject the matchup. Today's behaviour, and the right default. Reject, /// Ignore unknown members; a team with no known members is still an error. Skip, } ``` Even without that, one line in `predict_outcome`'s docs — *"every key must already be known to the history; pre-filter with `lookup`/`current_skill` if that is not guaranteed"* — would have saved the day. The doc currently describes what the function returns, not what it requires. ## Why this is worth more than the sum of its parts This crate's recurring defect, in its own issue history, is **a public surface that reports a plausible constant**. This is the caller-side twin: an error so opaque that the natural handling turns it into one. A neutral 0.5 from a prediction function is indistinguishable from a real answer, so the failure is invisible to every test that does not assert on variation. Our regression test now pins it, and it is worth stating what it took to find: no exception, no log line, no failing test — only a parameter sweep that produced suspiciously identical numbers. ## Concrete asks, smallest first 1. Include the key in `UnknownKey` (or its `Display`). 2. Document the precondition on `predict_outcome`, `predict_quality` and `expected_information_gain`. 3. Consider an opt-in `UnknownKeys::Skip`, which would also remove the double resolution.

logaritmisk referenced this issue from a commit

2026-09-07 21:41:11 +00:00

feat!: name the unknown key, expose tail probabilities, flag short fits

logaritmisk commented

2026-09-07 21:41:30 +00:00

Asks 1 and 2 are done in c12bc83. Ask 3 is deliberately still open — see below.

Thank you for the write-up. "No exception, no log line, no failing test — only a parameter sweep that produced suspiciously identical numbers" is the most useful sentence in this tracker, and the framing of it as the caller-side twin of this crate's own recurring defect is exactly right.

For the record: the 0.4.0 change that turned unknown keys into an error was mine, and shipping an error too anonymous to act on is what turned a correct fix into a day of silent breakage. That is the part worth apologising for.

1. The error names the key

UnknownKey {
    team: usize,
    member: usize,
    key: String,      // the key's Debug rendering
}

The prediction methods now carry a K: Debug bound in order to fill it. Its Display also says what to do:

team 1, member 0: no skill recorded for key "never_seen" (every key must
already be known to the history; pre-filter with `lookup` or `current_skill`
if that is not guaranteed)

unknown_key_names_the_key_it_could_not_find asserts both the field and the rendering, so neither can quietly regress.

2. The precondition is documented

predict_quality, predict_win_probabilities, predict_outcome, predict_ranking and expected_information_gain all gained a # Preconditions section. It says the keys must be known, points at lookup/current_skill, and — because this is the part that actually bit — names the failure mode:

treating the error as "no information" and substituting a neutral value turns a whole-team miss into a plausible constant, which is invisible to any test that does not assert on variation.

The README says it too, since that is where a new consumer looks first.

3. `UnknownKeys::Skip` — not done, and I want to check the shape with you

This is the one that needs a decision rather than an afternoon, so I would rather ask than guess.

The mode itself is easy. What is not obvious is where it lives. Predictions currently take no options struct (only expected_information_gain sees GameOptions), so Skip means one of:

a parallel set of methods — doubles the surface for one flag;
an options argument on all five — breaking, and heavy for the common case;
a builder-level default on History, which makes it a property of the fit rather than the query, and that seems wrong: whether an unknown key is fatal is a property of the question being asked, not of the model.

Also worth settling: you noted Skip would remove the double key resolution. It would — but only if a team that ends up empty is still an error, which your proposal already says. Worth confirming that is the semantics you want, since "skip unknowns, and a team of nothing but unknowns predicts from the prior" is a defensible alternative reading and a very different one.

Leaving this issue open for that. If the pre-filter is working acceptably for now, I would rather get the shape right than ship a third mode nobody wants.

Note on upgrading

This is a breaking release — UnknownKey gained a field, so any match on it needs updating, and the K: Debug bound is new. Both should be mechanical for lester-stash.

Asks 1 and 2 are done in `c12bc83`. Ask 3 is deliberately still open — see below. Thank you for the write-up. "No exception, no log line, no failing test — only a parameter sweep that produced suspiciously identical numbers" is the most useful sentence in this tracker, and the framing of it as the caller-side twin of this crate's own recurring defect is exactly right. For the record: the 0.4.0 change that turned unknown keys into an error was mine, and shipping an error too anonymous to act on is what turned a correct fix into a day of silent breakage. That is the part worth apologising for. ## 1. The error names the key ```rust UnknownKey { team: usize, member: usize, key: String, // the key's Debug rendering } ``` The prediction methods now carry a `K: Debug` bound in order to fill it. Its `Display` also says what to do: ``` team 1, member 0: no skill recorded for key "never_seen" (every key must already be known to the history; pre-filter with `lookup` or `current_skill` if that is not guaranteed) ``` `unknown_key_names_the_key_it_could_not_find` asserts both the field and the rendering, so neither can quietly regress. ## 2. The precondition is documented `predict_quality`, `predict_win_probabilities`, `predict_outcome`, `predict_ranking` and `expected_information_gain` all gained a `# Preconditions` section. It says the keys must be known, points at `lookup`/`current_skill`, and — because this is the part that actually bit — names the failure mode: > treating the error as "no information" and substituting a neutral value turns a whole-team miss into a plausible constant, which is invisible to any test that does not assert on variation. The README says it too, since that is where a new consumer looks first. ## 3. `UnknownKeys::Skip` — not done, and I want to check the shape with you This is the one that needs a decision rather than an afternoon, so I would rather ask than guess. The mode itself is easy. What is not obvious is where it lives. Predictions currently take no options struct (only `expected_information_gain` sees `GameOptions`), so `Skip` means one of: - a parallel set of methods — doubles the surface for one flag; - an options argument on all five — breaking, and heavy for the common case; - a builder-level default on `History`, which makes it a property of the fit rather than the query, and that seems wrong: whether an unknown key is fatal is a property of the *question being asked*, not of the model. Also worth settling: you noted `Skip` would remove the double key resolution. It would — but only if a team that ends up empty is still an error, which your proposal already says. Worth confirming that is the semantics you want, since "skip unknowns, and a team of nothing but unknowns predicts from the prior" is a defensible alternative reading and a very different one. Leaving this issue open for that. If the pre-filter is working acceptably for now, I would rather get the shape right than ship a third mode nobody wants. ## Note on upgrading This is a breaking release — `UnknownKey` gained a field, so any `match` on it needs updating, and the `K: Debug` bound is new. Both should be mechanical for lester-stash.

logaritmisk added the api bug decision labels 2026-09-07 21:41:39 +00:00

logaritmisk referenced this issue from a commit

2026-09-07 23:28:54 +00:00

feat: add UnknownKeys::Prior, and explain why there is no Skip

logaritmisk closed this issue

2026-09-07 23:28:54 +00:00

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: logaritmisk/trueskill-tt#44

UnknownKey reports indices, not the key — and rejecting the whole matchup is easy to swallow into a plausible constant #44