Files
trueskill-tt/tests/convergence_strictness.rs
T
logaritmiskandClaude Opus 5 8dff7513f7 fix!: seal ConstantDrift's field so gamma can be validated
`gamma` enters only as `gamma * gamma`, so the sign was squared away:
measured against the old public-field form, `ConstantDrift(-0.0833)`
produced results bit identical to `ConstantDrift(0.0833)`. The sign was
neither rejected nor honoured — it vanished.

It could not be checked while the field was a public tuple position,
because there was nothing to intercept. Validating inside
`variance_for_elapsed` would have been worse: it runs in the sweep, so a
construction-time mistake would panic mid-inference, and `Gaussian::from_ms`
is a worked example of why that is the wrong place — rejecting NaN there
turned the NonFiniteResult reporting path into a crash.

So `ConstantDrift::new` is the only way in and it checks, with `gamma()`
to read the value back. 129 call sites rewritten across src, tests,
benches, examples and the README. The dated plan and spec documents under
docs/superpowers are left alone: they record what was built at the time,
and rewriting them would falsify that.

tests/constructor_validation.rs is the more valuable half. This defect
class was closed three times in one session and reopened twice, because
each fix validated the layer it had just touched and inferred the rest —
`HistoryBuilder`, then `Game`'s own entry points, then the constructors
beneath both. A per-site fix cannot notice the site nobody thought of, so
that file enumerates every public entry point taking a magnitude and
asserts each refuses negative and non-finite values.

It found an eleventh defect on its first run: `HistoryBuilder::score_sigma`
accepted infinity, because `inf > 0.0` is true and the assert only tested
positivity. Fixed, and its own `should_panic` message updated to match.

`Gaussian::from_ms` is deliberately exempt from the non-finite half, for
the reason above: a broken fit produces a NaN sigma legitimately and
`converge` must be allowed to report it.

The convergence-level drift-variance check stays and is now tested through
a custom `Drift` implementation, since `ConstantDrift` can no longer reach
it. That check is the only thing standing between a third-party `Drift`
and a NaN fit.

BREAKING CHANGE: `ConstantDrift`'s field is private. Replace
`ConstantDrift(x)` with `ConstantDrift::new(x)`, and `drift().0` with
`drift().gamma()`. `HistoryBuilder::score_sigma` now rejects infinity.

Closes #65

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011hcFjNDmHXZF8URGLku5zZ
2026-09-09 19:11:57 +02:00

153 lines
4.5 KiB
Rust

//! Stopping short of convergence is an error, not a flag on a success.
//!
//! A fit that hits `max_iter` is wrong by a little: every rating is finite,
//! the ordering looks sensible, and nothing in the numbers says they were
//! still moving. When that was `Ok` with `converged: false`, detecting it was
//! opt-in and `let _ = h.converge()` was the natural way to opt out — which is
//! how a real defect once hid in this crate's own suite.
use smallvec::smallvec;
use trueskill_tt::{
ConstantDrift, ConvergenceOptions, Event, History, InferenceError, Member, Outcome, Team,
};
type H = History<i64, ConstantDrift, trueskill_tt::NullObserver, &'static str>;
fn duel(a: &'static str, b: &'static str, t: i64) -> Event<i64, &'static str> {
Event {
time: t,
teams: smallvec![
Team::with_members([Member::new(a)]),
Team::with_members([Member::new(b)]),
],
outcome: Outcome::scores([3.0, 1.0]),
}
}
fn capped(max_iter: usize) -> H {
History::builder()
.mu(0.0)
.sigma(6.0)
.beta(1.0)
.score_sigma(2.0)
.drift(ConstantDrift::new(0.5))
.convergence(ConvergenceOptions {
max_iter,
epsilon: 1e-13,
alpha: 1.0,
})
.build()
}
fn fill(h: &mut H) {
h.add_events((1..=6).map(|t| duel("a", "b", t)).collect::<Vec<_>>())
.unwrap();
}
#[test]
fn hitting_the_cap_is_an_error() {
let mut h = capped(1);
fill(&mut h);
let err = h.converge().unwrap_err();
match err {
InferenceError::NotConverged {
iterations,
final_step,
epsilon,
} => {
assert_eq!(iterations, 1);
assert!(
final_step.0 > epsilon || final_step.1 > epsilon,
"{final_step:?}"
);
}
other => panic!("expected NotConverged, got {other:?}"),
}
}
/// The message has to name what to do about it, since the fit looks fine.
#[test]
fn the_error_says_how_to_fix_it() {
let mut h = capped(1);
fill(&mut h);
let text = h.converge().unwrap_err().to_string();
assert!(text.contains("did not converge in 1 iterations"), "{text}");
assert!(text.contains("max_iter"), "{text}");
assert!(text.contains("alpha"), "{text}");
}
/// The escape hatch: a deliberately capped fit is still reachable.
#[test]
fn converge_partial_returns_the_short_fit() {
let mut h = capped(1);
fill(&mut h);
let report = h.converge_partial().unwrap();
assert_eq!(report.iterations, 1);
assert!(!report.converged);
assert!(h.current_skill(&"a").is_some());
}
/// Both agree when the fit does converge, so the strict path costs nothing.
#[test]
fn the_two_agree_on_a_converged_fit() {
let mut strict = capped(20_000);
fill(&mut strict);
let a = strict.converge().unwrap();
let mut partial = capped(20_000);
fill(&mut partial);
let b = partial.converge_partial().unwrap();
assert!(a.converged && b.converged);
assert_eq!(a.iterations, b.iterations);
assert_eq!(a.final_step, b.final_step);
}
/// The default cap must be high enough that an ordinary history clears it.
/// At the old value of 30 this history stopped short and said nothing.
#[test]
fn the_default_cap_clears_an_ordinary_history() {
let mut h: History<i64, ConstantDrift, _, String> = History::builder_with_key()
.mu(0.0)
.sigma(6.0)
.beta(1.0)
.score_sigma(2.0)
.drift(ConstantDrift::new(0.05))
.build();
let mut events = Vec::new();
for t in 0..20i64 {
for j in 0..8usize {
let k = (t as usize) * 8 + j;
events.push(Event {
time: t,
teams: smallvec![
Team::with_members([Member::new(format!("p{}", k % 100))]),
Team::with_members([Member::new(format!("p{}", (k + 37) % 100))]),
],
outcome: Outcome::scores([3.0, 1.0]),
});
}
}
h.add_events(events).unwrap();
let report = h
.converge()
.expect("an ordinary history must converge by default");
assert!(
report.iterations > 30,
"needed {} sweeps",
report.iterations
);
assert!(report.iterations < trueskill_tt::ITERATIONS);
}
/// An empty history converges trivially rather than erroring.
#[test]
fn an_empty_history_converges() {
let mut h = capped(1);
let report = h.converge().unwrap();
assert!(report.converged);
assert_eq!(report.iterations, 0);
}