From e4ff46f45cd8e858fb165444a40e797ea6104a64 Mon Sep 17 00:00:00 2001 From: Anders Olsson Date: Fri, 12 Jun 2026 20:27:47 +0200 Subject: [PATCH] fix(gaussian): treat non-positive precision as improper in mu()/sigma() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit EP message cancellation can leave a Gaussian's precision (pi) a tiny negative value — round-off of exactly zero. mu()/sigma() only special-cased pi == 0, so sigma() computed 1/sqrt(pi) = NaN for pi < 0. That NaN flowed through the moment-space Sub in the game diff-chain and poisoned every skill in the slice once it grew past ~75 competitors, making converge() return all-NaN on real-scale histories (regression vs 0.1.0, which stored sigma directly). Guard pi <= 0.0 in both accessors (improper Gaussian: mu 0, sigma infinite), matching the existing pi == 0 handling. Co-Authored-By: Claude Opus 4.8 (1M context) --- src/gaussian.rs | 33 +++++++++++- tests/large_history_converges_finite.rs | 71 +++++++++++++++++++++++++ 2 files changed, 102 insertions(+), 2 deletions(-) create mode 100644 tests/large_history_converges_finite.rs diff --git a/src/gaussian.rs b/src/gaussian.rs index 7b88cd5..1d8d705 100644 --- a/src/gaussian.rs +++ b/src/gaussian.rs @@ -53,7 +53,11 @@ impl Gaussian { #[inline] pub fn mu(&self) -> f64 { - if self.pi == 0.0 { + // A non-positive precision is an improper (uninformative) Gaussian — its mean is + // undefined. Treat it like `pi == 0` and return 0. EP message cancellation can land + // `pi` on a tiny negative value (round-off of exactly zero); without this guard + // `tau / pi` would yield a spurious finite mean. + if self.pi <= 0.0 { 0.0 } else { self.tau / self.pi @@ -62,7 +66,10 @@ impl Gaussian { #[inline] pub fn sigma(&self) -> f64 { - if self.pi == 0.0 { + // A non-positive precision is improper → infinite standard deviation. Guarding + // `pi <= 0.0` (not just `== 0.0`) keeps `1.0 / pi.sqrt()` from returning NaN when EP + // cancellation produces a tiny negative precision (round-off of exactly zero). + if self.pi <= 0.0 { f64::INFINITY } else if self.pi.is_infinite() { 0.0 @@ -174,6 +181,28 @@ impl ops::Div for Gaussian { mod tests { use super::*; + #[test] + fn non_positive_precision_is_improper_not_nan() { + // EP message cancellation can leave `pi` a tiny negative (round-off of exactly zero). + // Such a Gaussian is improper/uninformative: mu() must be 0 and sigma() infinite, not + // NaN. A NaN here propagates through the moment-space `Sub` in the game chain and + // poisons every skill in the slice. + let tiny_neg = Gaussian::from_natural(-5.55e-17, -8.88e-16); + assert_eq!(tiny_neg.mu(), 0.0); + assert!(tiny_neg.sigma().is_infinite()); + + // A frankly-negative precision is treated the same way. + let neg = Gaussian::from_natural(-1.0, 2.0); + assert_eq!(neg.mu(), 0.0); + assert!(neg.sigma().is_infinite()); + + // Subtracting such a message must not produce NaN (the original failure path). + let proper = Gaussian::from_ms(9.75, 1.256); + let diff = proper - tiny_neg; + assert!(diff.pi().is_finite() && !diff.pi().is_nan()); + assert!(diff.tau().is_finite() && !diff.tau().is_nan()); + } + #[test] fn test_add() { let n = Gaussian::from_ms(25.0, 25.0 / 3.0); diff --git a/tests/large_history_converges_finite.rs b/tests/large_history_converges_finite.rs new file mode 100644 index 0000000..98a5cc7 --- /dev/null +++ b/tests/large_history_converges_finite.rs @@ -0,0 +1,71 @@ +//! Regression: a single time slice with many distinct competitors must converge to finite +//! skills. Before the `pi <= 0` guard in `Gaussian::mu()/sigma()`, EP message cancellation +//! produced a tiny-negative precision whose `sigma() = 1/sqrt(pi)` was NaN, which the +//! moment-space `Sub` in the game chain propagated into every skill once the slice grew past +//! ~75 competitors (e.g. a real ranking dataset with hundreds of players). +use trueskill_tt::{ConstantDrift, ConvergenceOptions, EPSILON, History, ITERATIONS, NullObserver}; + +/// Tiny deterministic LCG — avoids a dev-dependency on `rand`. +struct Lcg(u64); +impl Lcg { + fn next(&mut self) -> u64 { + self.0 = self + .0 + .wrapping_mul(6364136223846793005) + .wrapping_add(1442695040888963407); + self.0 + } + fn below(&mut self, n: usize) -> usize { + (self.next() >> 33) as usize % n + } + fn coin(&mut self) -> bool { + self.next() & 1 == 0 + } +} + +fn nan_after_fit(players: usize) -> usize { + let mut h: History = History::builder_with_key() + .beta(1.0) + .sigma(6.0) + .drift(ConstantDrift(0.1)) + .convergence(ConvergenceOptions { + max_iter: ITERATIONS, + epsilon: EPSILON, + ..Default::default() + }) + .build(); + + let ids: Vec = (0..players).map(|i| format!("p{i:04}")).collect(); + let mut rng = Lcg(1); + for _ in 0..(players * 4) { + let a = rng.below(players); + let mut b = rng.below(players - 1); + if b >= a { + b += 1; + } + let (w, l) = if rng.coin() { (a, b) } else { (b, a) }; + h.record_winner(&ids[w], &ids[l], 0).unwrap(); + } + h.converge().unwrap(); + + ids.iter() + .filter(|id| { + h.current_skill(id.as_str()) + .map(|g| !g.mu().is_finite() || !g.sigma().is_finite()) + .unwrap_or(true) + }) + .count() +} + +#[test] +fn many_competitors_converge_to_finite_skills() { + // The NaN regression onset was between 70 and 80 competitors; 250 is comfortably past it + // and in the range of a real ranking dataset. + for players in [12usize, 75, 150, 250] { + assert_eq!( + nan_after_fit(players), + 0, + "{players}-competitor history produced NaN skills" + ); + } +}