From f4e2922d594053590d2029f7d4029a4b5b05e506 Mon Sep 17 00:00:00 2001 From: Anders Olsson Date: Tue, 4 Aug 2026 21:38:29 +0200 Subject: [PATCH] fix: reject ties without draw probability; never report NaN as converged MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A tie with `p_draw == 0.0` produced NaN posteriors in release builds and `converge()` reported `converged: true`, because every comparison against NaN is false and `tuple_gt` therefore read NaN as "below epsilon". Two independent defects, fixed together: - Ingestion now rejects tied outcomes when the draw probability is zero, promoting the existing `debug_assert!` in `Game::ranked_with_arena` to a real `InferenceError::TieWithoutDrawProbability`. Validation sits in `add_events_with_prior`, the chokepoint every route reaches — including `record_draw`, which bypasses `Outcome` entirely. - `converge()` treats a non-finite step as failure and returns `InferenceError::NonFiniteResult` rather than claiming convergence. Also in this change: - `History::converge()` on an empty history returned a `usize` underflow panic from `0..len()-1`; it now short-circuits to a zero-iteration report. - `Outcome::scores_with_sigma` no longer panics on a non-positive sigma; the value is validated at ingestion so callers get an error instead. - `InferenceError` gains `WrongOutcomeKind`, replacing the misuse of `MismatchedShape` for variant mismatches (which rendered as the nonsense "expected length 0, got 0"), and is now `#[non_exhaustive]`. Note `Outcome::winner(w, n)` for n >= 3 ties every loser, so those events now require a positive `p_draw`. They previously returned NaN. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01DnsaJg74eNSva3PJjK2eej --- src/error.rs | 41 ++++++++ src/game.rs | 28 ++++-- src/history.rs | 58 ++++++++++- src/lib.rs | 50 ++++++++++ src/outcome.rs | 18 +++- tests/degenerate_inputs.rs | 195 +++++++++++++++++++++++++++++++++++++ 6 files changed, 371 insertions(+), 19 deletions(-) create mode 100644 tests/degenerate_inputs.rs diff --git a/src/error.rs b/src/error.rs index 66d1e45..520e58d 100644 --- a/src/error.rs +++ b/src/error.rs @@ -1,6 +1,7 @@ use std::fmt; #[derive(Debug, Clone, PartialEq)] +#[non_exhaustive] pub enum InferenceError { /// Expected and actual lengths of some array-shaped input differ. MismatchedShape { @@ -8,15 +9,35 @@ pub enum InferenceError { expected: usize, got: usize, }, + /// An `Outcome` of the wrong variant was supplied for the requested inference. + WrongOutcomeKind { + context: &'static str, + expected: &'static str, + got: &'static str, + }, /// A probability value is outside `[0, 1]`. InvalidProbability { value: f64 }, /// A scalar parameter is outside its valid range. InvalidParameter { name: &'static str, value: f64 }, + /// An event contains tied teams, but the draw probability is zero. + /// + /// A zero draw probability asserts that draws cannot occur, so a tied + /// result has no representable likelihood. Configure a positive `p_draw` + /// (via `HistoryBuilder::p_draw` or `GameOptions::p_draw`) to admit ties. + TieWithoutDrawProbability { teams: (usize, usize) }, /// Convergence exceeded `max_iter` without falling below `epsilon`. ConvergenceFailed { last_step: (f64, f64), iterations: usize, }, + /// Inference produced a non-finite value (NaN or infinity). + /// + /// Indicates numerical breakdown; the resulting skills are meaningless + /// and must not be treated as a converged estimate. + NonFiniteResult { + context: &'static str, + step: (f64, f64), + }, /// Negative precision: a Gaussian with `pi < 0` slipped into an API call. NegativePrecision { pi: f64 }, } @@ -31,9 +52,29 @@ impl fmt::Display for InferenceError { } => { write!(f, "{kind}: expected length {expected}, got {got}") } + Self::WrongOutcomeKind { + context, + expected, + got, + } => { + write!(f, "{context}: expected {expected}, got {got}") + } Self::InvalidProbability { value } => { write!(f, "probability must be in [0, 1]; got {value}") } + Self::TieWithoutDrawProbability { teams } => { + write!( + f, + "teams {} and {} are tied, but p_draw is 0.0; set a positive draw probability to admit ties", + teams.0, teams.1 + ) + } + Self::NonFiniteResult { context, step } => { + write!( + f, + "{context}: inference produced a non-finite result (step = {step:?})" + ) + } Self::InvalidParameter { name, value } => { write!(f, "{name} is invalid: {value}") } diff --git a/src/game.rs b/src/game.rs index 1b9ede3..e00f29f 100644 --- a/src/game.rs +++ b/src/game.rs @@ -458,11 +458,18 @@ impl> Game<'_, T, D> { let ranks = outcome .as_ranks() - .ok_or(crate::InferenceError::MismatchedShape { - kind: "Game::ranked requires Outcome::Ranked", - expected: 0, - got: 0, + .ok_or(crate::InferenceError::WrongOutcomeKind { + context: "Game::ranked", + expected: "Outcome::Ranked", + got: "Outcome::Scored", })?; + + if options.p_draw == 0.0 + && let Some(tied) = crate::first_tied_pair(ranks) + { + return Err(crate::InferenceError::TieWithoutDrawProbability { teams: tied }); + } + let max_rank = ranks.iter().copied().max().unwrap_or(0) as f64; let result: Vec = ranks.iter().map(|&r| max_rank - r as f64).collect(); let teams_owned: Vec>> = teams.iter().map(|t| t.to_vec()).collect(); @@ -497,10 +504,10 @@ impl> Game<'_, T, D> { } let scores = outcome .as_scores() - .ok_or(crate::InferenceError::MismatchedShape { - kind: "Game::scored requires Outcome::Scored", - expected: 0, - got: 0, + .ok_or(crate::InferenceError::WrongOutcomeKind { + context: "Game::scored", + expected: "Outcome::Scored", + got: "Outcome::Ranked", })? .to_vec(); let teams_owned: Vec>> = teams.iter().map(|t| t.to_vec()).collect(); @@ -1124,7 +1131,10 @@ mod tests { &GameOptions::default(), ) .unwrap_err(); - assert!(matches!(err, crate::InferenceError::MismatchedShape { .. })); + assert!(matches!( + err, + crate::InferenceError::WrongOutcomeKind { .. } + )); } #[test] diff --git a/src/history.rs b/src/history.rs index 89f0e16..fbb050e 100644 --- a/src/history.rs +++ b/src/history.rs @@ -220,6 +220,10 @@ impl, O: Observer, K: Eq + Hash + Clone> History (f64, f64) { let mut step = (0.0, 0.0); + if self.time_slices.is_empty() { + return step; + } + competitor::clean(self.agents.values_mut(), false); for j in (0..self.time_slices.len() - 1).rev() { @@ -435,6 +439,18 @@ impl, O: Observer, K: Eq + Hash + Clone> History = SmallVec::new(); @@ -444,8 +460,24 @@ impl, O: Observer, K: Eq + Hash + Clone> History, O: Observer, K: Eq + Hash + Clone> History, O: Observer, K: Eq + Hash + Clone> History { let resolved = sigma.unwrap_or(self.score_sigma); - debug_assert!( - resolved > 0.0, - "resolved score_sigma must be > 0.0 (got {resolved})" - ); + if !(resolved > 0.0) { + return Err(InferenceError::InvalidParameter { + name: "score_sigma", + value: resolved, + }); + } + kinds.push(EventKind::Scored { score_sigma: resolved, }); diff --git a/src/lib.rs b/src/lib.rs index f40514c..cdacbfa 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -184,6 +184,56 @@ pub(crate) fn tuple_gt(t: (f64, f64), e: f64) -> bool { t.0 > e || t.1 > e } +/// Whether a convergence step is finite in both components. +/// +/// A NaN step means EP broke down numerically. Because every comparison +/// against NaN is false, `tuple_gt` reads NaN as "below epsilon" — so +/// convergence checks must test finiteness explicitly rather than inferring +/// success from `!tuple_gt(..)`. +pub(crate) fn step_is_finite(t: (f64, f64)) -> bool { + t.0.is_finite() && t.1.is_finite() +} + +/// Whether a step counts as converged: finite *and* within `epsilon`. +pub(crate) fn step_converged(t: (f64, f64), epsilon: f64) -> bool { + step_is_finite(t) && !tuple_gt(t, epsilon) +} + +/// Indices of the first pair of teams sharing a rank, if any. +/// +/// A tie is only representable when the draw probability is positive: with +/// `p_draw == 0.0` the truncation margin collapses to zero and the two-sided +/// tie update evaluates `0/0`. Callers use this to reject such events before +/// they reach inference. +pub(crate) fn first_tied_pair(ranks: &[u32]) -> Option<(usize, usize)> { + for (i, a) in ranks.iter().enumerate() { + for (j, b) in ranks.iter().enumerate().skip(i + 1) { + if a == b { + return Some((i, j)); + } + } + } + + None +} + +/// As `first_tied_pair`, but over the engine's internal `f64` outputs. +/// +/// Ranks reach the engine already converted to descending `f64` outputs, and +/// `Game` decides a tie by exact equality of those values — so this mirrors +/// the comparison inference itself performs. +pub(crate) fn first_tied_output(outputs: &[f64]) -> Option<(usize, usize)> { + for (i, a) in outputs.iter().enumerate() { + for (j, b) in outputs.iter().enumerate().skip(i + 1) { + if a == b { + return Some((i, j)); + } + } + } + + None +} + pub(crate) fn sort_time(xs: &[T], reverse: bool) -> Vec { let mut x: Vec<(usize, T)> = xs.iter().enumerate().map(|(i, &t)| (i, t)).collect(); diff --git a/src/outcome.rs b/src/outcome.rs index 51a78ac..17777d1 100644 --- a/src/outcome.rs +++ b/src/outcome.rs @@ -57,9 +57,11 @@ impl Outcome { /// Explicit per-team continuous scores with a per-event noise override. /// - /// `sigma` must be `> 0.0`; debug-asserts otherwise. + /// `sigma` must be `> 0.0`. Constructing an `Outcome` with a non-positive + /// or NaN sigma is allowed; the value is rejected with + /// `InferenceError::InvalidParameter` when the event is ingested, so + /// callers get an error rather than a panic. pub fn scores_with_sigma>(scores: I, sigma: f64) -> Self { - debug_assert!(sigma > 0.0, "score_sigma must be > 0.0 (got {sigma})"); Self::Scored { scores: scores.into_iter().collect(), sigma: Some(sigma), @@ -169,9 +171,15 @@ mod tests { } } + /// Construction accepts any sigma; the value is validated at ingestion so + /// callers receive an `InferenceError` rather than a panic. See + /// `tests/degenerate_inputs.rs::scored_event_rejects_non_positive_sigma`. #[test] - #[should_panic(expected = "score_sigma must be > 0.0")] - fn scores_with_sigma_rejects_zero() { - let _ = Outcome::scores_with_sigma([3.0, 1.0], 0.0); + fn scores_with_sigma_defers_validation_to_ingestion() { + let o = Outcome::scores_with_sigma([3.0, 1.0], 0.0); + match o { + Outcome::Scored { sigma, .. } => assert_eq!(sigma, Some(0.0)), + Outcome::Ranked(_) => panic!("expected Scored variant"), + } } } diff --git a/tests/degenerate_inputs.rs b/tests/degenerate_inputs.rs new file mode 100644 index 0000000..4ad7ebd --- /dev/null +++ b/tests/degenerate_inputs.rs @@ -0,0 +1,195 @@ +//! Degenerate, boundary, and error-path coverage. +//! +//! These run in both debug and release: the defects they pin were all +//! guarded only by `debug_assert!`, so a debug-only suite never saw them. + +use trueskill_tt::{ + ConstantDrift, ConvergenceOptions, Game, GameOptions, Gaussian, History, InferenceError, + Outcome, Rating, +}; + +type R = Rating; + +fn rating() -> R { + R::new( + Gaussian::from_ms(25.0, 25.0 / 3.0), + 25.0 / 6.0, + ConstantDrift(25.0 / 300.0), + ) +} + +fn assert_finite(g: Gaussian, what: &str) { + assert!( + g.mu().is_finite() && g.sigma().is_finite(), + "{what} must be finite, got mu={} sigma={}", + g.mu(), + g.sigma() + ); +} + +#[test] +fn record_draw_without_draw_probability_is_rejected() { + let mut h = History::default(); + let err = h.record_draw(&"a", &"b", 1).unwrap_err(); + assert!(matches!( + err, + InferenceError::TieWithoutDrawProbability { .. } + )); +} + +#[test] +fn builder_draw_without_draw_probability_is_rejected() { + let mut h = History::default(); + let err = h + .event(1) + .team(["a"]) + .team(["b"]) + .draw() + .commit() + .unwrap_err(); + assert!(matches!( + err, + InferenceError::TieWithoutDrawProbability { .. } + )); +} + +#[test] +fn draw_with_positive_draw_probability_is_finite() { + let mut h = History::builder().p_draw(0.25).build(); + h.record_draw(&"a", &"b", 1).unwrap(); + let report = h.converge().unwrap(); + + assert_finite(h.current_skill("a").unwrap(), "drawn competitor skill"); + assert_finite(h.current_skill("b").unwrap(), "drawn competitor skill"); + assert!(report.log_evidence.is_finite()); + assert!(report.converged); +} + +#[test] +fn game_ranked_rejects_tie_without_draw_probability() { + let a = [rating()]; + let b = [rating()]; + let teams: Vec<&[R]> = vec![&a, &b]; + let err = Game::ranked(&teams, Outcome::draw(2), &GameOptions::default()).unwrap_err(); + assert!(matches!( + err, + InferenceError::TieWithoutDrawProbability { .. } + )); +} + +/// `Outcome::winner(w, n)` ties every loser, so any n >= 3 free-for-all hits +/// the tie path even though the caller never asked for a draw. +#[test] +fn winner_of_three_or_more_requires_draw_probability() { + let a = [rating()]; + let b = [rating()]; + let c = [rating()]; + let teams: Vec<&[R]> = vec![&a, &b, &c]; + + let err = Game::ranked(&teams, Outcome::winner(0, 3), &GameOptions::default()).unwrap_err(); + assert!(matches!( + err, + InferenceError::TieWithoutDrawProbability { .. } + )); + + let opts = GameOptions { + p_draw: 0.1, + ..GameOptions::default() + }; + let game = Game::ranked(&teams, Outcome::winner(0, 3), &opts).unwrap(); + for team in game.posteriors() { + for skill in team { + assert_finite(skill, "3-team winner posterior"); + } + } +} + +#[test] +fn full_ranking_without_ties_needs_no_draw_probability() { + let a = [rating()]; + let b = [rating()]; + let c = [rating()]; + let teams: Vec<&[R]> = vec![&a, &b, &c]; + let game = Game::ranked(&teams, Outcome::ranking([0, 1, 2]), &GameOptions::default()).unwrap(); + + for team in game.posteriors() { + for skill in team { + assert_finite(skill, "strict ranking posterior"); + } + } +} + +#[test] +fn empty_history_converges_trivially() { + let mut h = History::default(); + let report = h.converge().unwrap(); + assert_eq!(report.iterations, 0); + assert!(report.converged); +} + +#[test] +fn empty_event_stream_then_converge() { + let mut h = History::default(); + h.add_events(std::iter::empty()).unwrap(); + let report = h.converge().unwrap(); + assert_eq!(report.iterations, 0); +} + +#[test] +fn empty_history_queries_do_not_panic() { + let h = History::default(); + assert!(h.learning_curves().is_empty()); + assert!(h.learning_curve("nobody").is_empty()); + assert!(h.current_skill("nobody").is_none()); +} + +#[test] +fn single_event_history_converges() { + let mut h = History::default(); + h.record_winner(&"a", &"b", 1).unwrap(); + let report = h.converge().unwrap(); + assert!(report.converged); + assert_finite(h.current_skill("a").unwrap(), "single-event skill"); +} + +#[test] +fn scored_event_rejects_non_positive_sigma() { + let mut h = History::builder().score_sigma(2.0).build(); + let err = h + .event(1) + .team(["a"]) + .team(["b"]) + .scores_with_sigma([3.0, 1.0], f64::NAN) + .commit() + .unwrap_err(); + assert!(matches!( + err, + InferenceError::InvalidParameter { + name: "score_sigma", + .. + } + )); +} + +#[test] +fn convergence_reports_are_finite_across_many_teams() { + let opts = GameOptions { + p_draw: 0.1, + convergence: ConvergenceOptions::default(), + ..GameOptions::default() + }; + let holders: Vec<[R; 1]> = (0..12).map(|_| [rating()]).collect(); + let teams: Vec<&[R]> = holders.iter().map(|t| t.as_slice()).collect(); + let game = Game::ranked(&teams, Outcome::ranking(0..12), &opts).unwrap(); + + assert!( + game.log_evidence().is_finite(), + "12-team log-evidence must be finite, got {}", + game.log_evidence() + ); + for team in game.posteriors() { + for skill in team { + assert_finite(skill, "12-team posterior"); + } + } +}