From ab23476aaf51ad8ea4878d462580ad12c6fd6b7b Mon Sep 17 00:00:00 2001 From: Anders Olsson Date: Wed, 9 Sep 2026 17:48:41 +0200 Subject: [PATCH] fix!: validate the constructors below HistoryBuilder MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 0.8.0 closed the sign-absorption defect at `HistoryBuilder::mu/sigma/beta` and at both ingestion paths. It was still open one layer down, in the constructors those paths call. Measured, all bit identical to their positive counterparts: Gaussian::from_ms(25.0, -8.33) == from_ms(25.0, +8.33) Rating::new(_, -4.17, _) == Rating::new(_, +4.17, _) ConstantDrift(-0.0833) == ConstantDrift(+0.0833) sigma, beta and gamma enter only as squares, so the sign vanished without comment. Worst of the set: `Rating::new(_, NaN, _)` reached `Game::ranked` which returned **Ok** carrying `Gaussian { pi: NaN, tau: NaN }` — no `converge` on that path to catch it. `from_ms` and `Rating::new` now reject. `ConstantDrift` cannot: the field is public and positional, so there is no constructor to intercept, and sealing it would break every `ConstantDrift(x)` for a case whose resulting model is perfectly valid. Documented instead. Its non-finite half IS rejected — `converge` validates the drift variance each competitor accumulates, which also covers a custom `Drift` impl. Two things the tests caught that I had wrong: NaN sigma must PASS `from_ms`. My first version rejected it, and two existing tests went red immediately: a broken fit legitimately produces a NaN sigma from `sqrt` of a negative truncated variance, and the design is to propagate that to `NonFiniteResult`. Rejecting it turned the reporting path into a panic inside inference. Written as `sigma >= 0.0 || sigma.is_nan()` so the intent is explicit rather than hidden in a negated comparison. Very small sigma is also not rejected, and that is deliberate: `approx` produces small truncated sigmas legitimately. `pi = 1/sigma^2` leaves f64's range below ~1.5e-154 and `tau = mu*pi` overflows sooner, at a threshold that depends on mu — so there is a band where pi is finite and only tau is not. Both land on the existing point-mass representation. Documented, including that such a Gaussian is not equal to itself and can make two identical declarations report as conflicting. BREAKING CHANGE: `Gaussian::from_ms` panics on a negative sigma, and `Rating::new` panics unless beta is finite and non-negative. `converge` returns `InvalidParameter` for a non-finite drift variance. Closes #61 Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_011hcFjNDmHXZF8URGLku5zZ --- src/drift.rs | 17 +++++++++ src/gaussian.rs | 36 +++++++++++++++++++ src/history.rs | 24 +++++++++++++ src/rating.rs | 17 +++++++++ tests/validation.rs | 85 +++++++++++++++++++++++++++++++++++++++++++++ 5 files changed, 179 insertions(+) diff --git a/src/drift.rs b/src/drift.rs index c751624..8f89415 100644 --- a/src/drift.rs +++ b/src/drift.rs @@ -21,6 +21,23 @@ pub trait Drift: Copy + Debug + Send + Sync { /// /// For `Time = i64`: variance added is `(to - from) * gamma^2`. /// For `Time = Untimed`: elapsed is always 0, so drift is always 0. +/// +/// # The sign of `gamma` is not meaningful +/// +/// `gamma` enters only as `gamma * gamma`, so `ConstantDrift(-0.05)` produces +/// results **bit identical** to `ConstantDrift(0.05)`. That is the same +/// sign-absorption `HistoryBuilder::sigma`, `HistoryBuilder::beta`, +/// `Gaussian::from_ms` and `Rating::new` all reject outright. +/// +/// It is not rejected here because the field is public and positional, so +/// there is no constructor to intercept — sealing it would break every +/// `ConstantDrift(x)` in existence for a case whose *resulting model* is +/// perfectly valid, just not the one a caller writing a minus sign expected. +/// +/// A non-finite `gamma` is a different matter and **is** rejected: +/// `History::converge` validates the drift variance each competitor actually +/// accumulates, which also covers a custom [`Drift`] implementation, and +/// reports `InferenceError::InvalidParameter`. #[derive(Clone, Copy, Debug)] pub struct ConstantDrift(pub f64); diff --git a/src/gaussian.rs b/src/gaussian.rs index c1d76be..08ba486 100644 --- a/src/gaussian.rs +++ b/src/gaussian.rs @@ -18,8 +18,44 @@ pub struct Gaussian { impl Gaussian { /// Construct from mean and standard deviation. + /// + /// # Panics + /// + /// Panics if `sigma` is negative. NaN is deliberately allowed through: a + /// broken fit produces one, and `converge` reports that as + /// `NonFiniteResult` rather than panicking mid-inference. + /// + /// A negative sigma used to be accepted and returned results **bit + /// identical** to its absolute value, because sigma only ever enters as + /// `sigma * sigma`. The sign was not rejected and not honoured; it simply + /// vanished. That is the same defect `HistoryBuilder::sigma`, + /// `HistoryBuilder::beta` and `Member::with_drift_scale` already reject. + /// + /// # Very small sigma + /// + /// `pi = 1 / sigma^2` leaves `f64`'s range below about `1.5e-154`, and + /// `tau = mu * pi` overflows sooner still — at a threshold that depends on + /// `mu`, so there is a band where `pi` is finite and only `tau` is not. + /// Both land on the same point-mass representation the `sigma == 0.0` + /// branch produces, and a point mass with a non-zero mean has `mu() = NaN`, + /// because `tau / pi` is `inf / inf`. + /// + /// This is not rejected, because `approx` legitimately produces a very + /// small truncated sigma and inference must not panic. It is worth knowing + /// that such a `Gaussian` is not equal to itself, so two identical + /// declarations of one can be reported as conflicting. #[must_use] pub const fn from_ms(mu: f64, sigma: f64) -> Self { + // NaN is admitted on purpose. A broken fit legitimately produces a NaN + // sigma — `sqrt` of a negative truncated variance — and the design is + // to propagate that to `converge`'s `NonFiniteResult` guard, not to + // panic inside inference. Rejecting it here turned that reporting path + // into a crash, which two tests caught immediately. + assert!( + sigma >= 0.0 || sigma.is_nan(), + "sigma must not be negative; it is only ever squared, so a negative \ + value would silently behave as its absolute value" + ); if sigma == f64::INFINITY { Self { pi: 0.0, tau: 0.0 } } else if sigma == 0.0 { diff --git a/src/history.rs b/src/history.rs index dafc48b..fa1bf70 100644 --- a/src/history.rs +++ b/src/history.rs @@ -1644,6 +1644,30 @@ impl, O: Observer, K: Eq + Hash + Clone> History` and cannot inspect + // an arbitrary implementation. Validate what it actually produces + // instead, which also covers a custom impl. + // + // `ConstantDrift` returns `elapsed * gamma * gamma`, so a negative + // gamma is squared away: measured, `ConstantDrift(-0.0833)` gave + // results **bit identical** to `+0.0833`, the same sign-absorption + // defect already rejected for `sigma` and `beta`. A non-finite gamma + // poisons every posterior derived from it. + for slice in &self.time_slices { + for (agent, elapsed) in slice.appearances() { + let drift = self.agents[agent] + .rating + .drift_variance_for_elapsed(elapsed); + if !drift.is_finite() || drift < 0.0 { + return Err(InferenceError::InvalidParameter { + name: "drift variance", + value: drift, + }); + } + } + } + if self.time_slices.is_empty() { return Ok(ConvergenceReport { iterations: 0, diff --git a/src/rating.rs b/src/rating.rs index f589dc8..5de6b5c 100644 --- a/src/rating.rs +++ b/src/rating.rs @@ -23,7 +23,24 @@ pub struct Rating = ConstantDrift> { } impl> Rating { + /// # Panics + /// + /// Panics unless `beta` is finite and non-negative, matching + /// `HistoryBuilder::beta`. + /// + /// Zero is allowed and meaningful — performance is then exactly skill, and + /// the fit differs measurably from a positive beta rather than degenerating. + /// Negative is rejected because `beta` enters only as `beta^2`: measured, a + /// negative beta returned results **bit identical** to its absolute value, + /// and a NaN beta reached `Game::ranked`, which returned `Ok` carrying a + /// `Gaussian { pi: NaN, tau: NaN }` — there is no `converge` on that path to + /// catch it. pub fn new(prior: Gaussian, beta: f64, drift: D) -> Self { + assert!( + beta.is_finite() && beta >= 0.0, + "beta must be finite and non-negative (got {beta}); it is only ever \ + squared, so a negative value would silently behave as its absolute value" + ); Self { prior, beta, diff --git a/tests/validation.rs b/tests/validation.rs index 9ec4686..a798fdf 100644 --- a/tests/validation.rs +++ b/tests/validation.rs @@ -255,3 +255,88 @@ mod builder_parameters { ); } } + +/// The constructors below `HistoryBuilder`, which 0.8.0's validation did not +/// reach. +/// +/// `sigma`, `beta` and `gamma` all enter inference only as squares, so a +/// negative value behaves as its absolute value and the sign vanishes without +/// comment. Measured before these guards: `from_ms(25.0, -8.33)` and +/// `Rating::new(_, -4.17, _)` returned results bit identical to their positive +/// counterparts, and `Rating::new(_, NaN, _)` reached `Game::ranked`, which +/// returned `Ok` carrying `Gaussian { pi: NaN, tau: NaN }`. +mod constructor_parameters { + use trueskill_tt::{ConstantDrift, Gaussian, History, InferenceError, Rating}; + + #[test] + #[should_panic(expected = "sigma must not be negative")] + fn a_negative_sigma_is_rejected_by_from_ms() { + let _ = Gaussian::from_ms(25.0, -8.33); + } + + /// NaN must pass, and that is deliberate: a broken fit produces a NaN + /// sigma and `converge` reports it as `NonFiniteResult`. Rejecting it here + /// would turn reporting into a panic inside inference. + #[test] + fn a_nan_sigma_passes_through_from_ms() { + let g = Gaussian::from_ms(25.0, f64::NAN); + assert!(g.sigma().is_nan() || g.pi().is_nan()); + } + + #[test] + #[should_panic(expected = "beta must be finite and non-negative")] + fn a_negative_beta_is_rejected_by_rating_new() { + let _ = Rating::::new(Gaussian::default(), -4.17, ConstantDrift(0.0)); + } + + #[test] + #[should_panic(expected = "beta must be finite and non-negative")] + fn a_nan_beta_is_rejected_by_rating_new() { + let _ = + Rating::::new(Gaussian::default(), f64::NAN, ConstantDrift(0.0)); + } + + #[test] + fn a_zero_beta_is_accepted_by_rating_new() { + let _ = Rating::::new(Gaussian::default(), 0.0, ConstantDrift(0.0)); + } + + /// `HistoryBuilder::drift` is generic and cannot inspect an arbitrary + /// `Drift`, so the check is on the variance each competitor actually + /// accumulates. That also covers a custom implementation. + #[test] + fn a_non_finite_drift_is_rejected_at_convergence() { + for gamma in [f64::NAN, f64::INFINITY] { + let mut h = History::builder() + .mu(25.0) + .sigma(25.0 / 3.0) + .beta(25.0 / 6.0) + .drift(ConstantDrift(gamma)) + .build(); + h.record_winner(&"a", &"b", 1).unwrap(); + h.record_winner(&"a", &"b", 5).unwrap(); + let err = h.converge().unwrap_err(); + assert!( + matches!( + err, + InferenceError::InvalidParameter { + name: "drift variance", + .. + } + ), + "gamma {gamma}: {err:?}" + ); + } + } + + /// An ordinary drift is untouched. + #[test] + fn an_ordinary_drift_still_converges() { + let mut h = History::builder() + .drift(ConstantDrift(25.0 / 300.0)) + .build(); + h.record_winner(&"a", &"b", 1).unwrap(); + h.record_winner(&"a", &"b", 5).unwrap(); + assert!(h.converge().unwrap().converged); + } +}