`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
376 lines
12 KiB
Rust
376 lines
12 KiB
Rust
//! Input validation must hold in **release**, where `debug_assert!` is gone.
|
|
//!
|
|
//! The engine guards itself with `debug_assert!`, which documents invariants
|
|
//! but vanishes in the profile users actually ship. Anything reachable from the
|
|
//! public API has to be rejected with an `InferenceError` instead, at the
|
|
//! boundary, rather than becoming NaN or an out-of-bounds panic deep inside
|
|
//! `run_chain`.
|
|
//!
|
|
//! `GameOptions` and `ConvergenceOptions` both have public fields, so the
|
|
//! eager asserts on `HistoryBuilder` do not cover the `Game` constructors —
|
|
//! a caller can build the options struct directly.
|
|
|
|
use smallvec::smallvec;
|
|
use trueskill_tt::{
|
|
ConstantDrift, ConvergenceOptions, Event, Game, GameOptions, Gaussian, History, InferenceError,
|
|
Member, Outcome, Rating, Team,
|
|
};
|
|
|
|
type R = Rating<i64, ConstantDrift>;
|
|
|
|
fn rating() -> R {
|
|
R::new(
|
|
Gaussian::from_ms(25.0, 25.0 / 3.0),
|
|
25.0 / 6.0,
|
|
ConstantDrift::new(0.0),
|
|
)
|
|
}
|
|
|
|
fn options_with_alpha(alpha: f64) -> GameOptions {
|
|
GameOptions {
|
|
convergence: ConvergenceOptions {
|
|
alpha,
|
|
..ConvergenceOptions::default()
|
|
},
|
|
..GameOptions::default()
|
|
}
|
|
}
|
|
|
|
/// `alpha == 0.0` leaves every EP update unapplied, so inference silently
|
|
/// returns the priors — the worst possible failure, since the output looks
|
|
/// entirely reasonable.
|
|
#[test]
|
|
fn ranked_rejects_a_zero_damping_factor() {
|
|
let (a, b) = (rating(), rating());
|
|
let err = Game::<i64, _>::ranked(
|
|
&[&[a], &[b]],
|
|
Outcome::winner(0, 2),
|
|
&options_with_alpha(0.0),
|
|
)
|
|
.expect_err("alpha = 0 must be rejected");
|
|
assert!(
|
|
matches!(err, InferenceError::InvalidParameter { name: "alpha", .. }),
|
|
"got {err:?}"
|
|
);
|
|
}
|
|
|
|
#[test]
|
|
fn ranked_rejects_an_out_of_range_damping_factor() {
|
|
let (a, b) = (rating(), rating());
|
|
for alpha in [-0.5, 1.5, f64::NAN] {
|
|
let err = Game::<i64, _>::ranked(
|
|
&[&[a], &[b]],
|
|
Outcome::winner(0, 2),
|
|
&options_with_alpha(alpha),
|
|
)
|
|
.expect_err("alpha out of (0, 1] must be rejected");
|
|
assert!(
|
|
matches!(err, InferenceError::InvalidParameter { name: "alpha", .. }),
|
|
"alpha={alpha}: got {err:?}"
|
|
);
|
|
}
|
|
}
|
|
|
|
#[test]
|
|
fn scored_rejects_a_bad_damping_factor() {
|
|
let (a, b) = (rating(), rating());
|
|
let err = Game::<i64, _>::scored(
|
|
&[&[a], &[b]],
|
|
Outcome::scores([21.0, 9.0]),
|
|
&options_with_alpha(0.0),
|
|
)
|
|
.expect_err("alpha = 0 must be rejected");
|
|
assert!(
|
|
matches!(err, InferenceError::InvalidParameter { name: "alpha", .. }),
|
|
"got {err:?}"
|
|
);
|
|
}
|
|
|
|
/// Already covered by `Game::ranked`, asserted here so the release-mode
|
|
/// guarantee is stated in one place.
|
|
#[test]
|
|
fn ranked_rejects_an_out_of_range_draw_probability() {
|
|
let (a, b) = (rating(), rating());
|
|
for p_draw in [-0.5, 1.0, 1.5] {
|
|
let options = GameOptions {
|
|
p_draw,
|
|
..GameOptions::default()
|
|
};
|
|
assert!(
|
|
Game::<i64, _>::ranked(&[&[a], &[b]], Outcome::winner(0, 2), &options).is_err(),
|
|
"p_draw={p_draw} must be rejected"
|
|
);
|
|
}
|
|
}
|
|
|
|
#[test]
|
|
fn scored_rejects_a_non_positive_noise() {
|
|
let (a, b) = (rating(), rating());
|
|
for score_sigma in [0.0, -1.0, f64::NAN] {
|
|
let options = GameOptions {
|
|
score_sigma,
|
|
..GameOptions::default()
|
|
};
|
|
assert!(
|
|
Game::<i64, _>::scored(&[&[a], &[b]], Outcome::scores([21.0, 9.0]), &options).is_err(),
|
|
"score_sigma={score_sigma} must be rejected"
|
|
);
|
|
}
|
|
}
|
|
|
|
/// A tie with no draw probability makes the truncation margin zero and the
|
|
/// two-sided update evaluate 0/0. Ingestion must refuse it.
|
|
#[test]
|
|
fn ingestion_rejects_a_tie_without_a_draw_probability() {
|
|
let mut h = History::builder().p_draw(0.0).build();
|
|
let err = h
|
|
.add_events(vec![Event {
|
|
time: 0,
|
|
teams: smallvec![
|
|
Team::with_members([Member::new("a")]),
|
|
Team::with_members([Member::new("b")]),
|
|
],
|
|
outcome: Outcome::draw(2),
|
|
}])
|
|
.expect_err("a tie with p_draw = 0 must be rejected");
|
|
assert!(
|
|
matches!(err, InferenceError::TieWithoutDrawProbability { .. }),
|
|
"got {err:?}"
|
|
);
|
|
}
|
|
|
|
/// `Outcome::scores_with_sigma` documents that a non-positive sigma is
|
|
/// accepted at construction and rejected at ingestion.
|
|
#[test]
|
|
fn ingestion_rejects_a_non_positive_per_event_score_sigma() {
|
|
for sigma in [0.0, -1.0, f64::NAN] {
|
|
let mut h = History::builder().build();
|
|
let err = h
|
|
.add_events(vec![Event {
|
|
time: 0,
|
|
teams: smallvec![
|
|
Team::with_members([Member::new("a")]),
|
|
Team::with_members([Member::new("b")]),
|
|
],
|
|
outcome: Outcome::scores_with_sigma([21.0, 9.0], sigma),
|
|
}])
|
|
.expect_err("a non-positive per-event sigma must be rejected");
|
|
assert!(
|
|
matches!(err, InferenceError::InvalidParameter { .. }),
|
|
"sigma={sigma}: got {err:?}"
|
|
);
|
|
}
|
|
}
|
|
|
|
/// Per-team weights must match that team's membership. The top-level length
|
|
/// checks in ingestion do not cover the inner dimension.
|
|
#[test]
|
|
fn ingestion_rejects_weights_that_do_not_match_their_team() {
|
|
let mut h = History::builder().build();
|
|
let mut team = Team::with_members([Member::new("a"), Member::new("b")]);
|
|
team.members[0].weight = 1.0;
|
|
|
|
let err = h
|
|
.event(0)
|
|
.team(["a", "b"])
|
|
.team(["c"])
|
|
// Three weights for a two-member team.
|
|
.weights([1.0, 1.0, 1.0])
|
|
.winner(0)
|
|
.commit()
|
|
.expect_err("a weight/member length mismatch must be rejected");
|
|
assert!(
|
|
matches!(err, InferenceError::MismatchedShape { .. }),
|
|
"got {err:?}"
|
|
);
|
|
}
|
|
|
|
/// `mu`, `sigma` and `beta` were the last unvalidated setters on
|
|
/// `HistoryBuilder`, next to `p_draw`, `score_sigma` and `convergence`, which
|
|
/// all assert eagerly.
|
|
///
|
|
/// Two of the rejected values are the quiet kind. A negative `sigma` or `beta`
|
|
/// enters inference only as its square, so it produced bit-identical results
|
|
/// to the positive value — the sign was dropped without comment.
|
|
mod builder_parameters {
|
|
use trueskill_tt::History;
|
|
|
|
#[test]
|
|
#[should_panic(expected = "mu must be finite")]
|
|
fn a_non_finite_mu_is_rejected() {
|
|
let _ = History::builder().mu(f64::NAN);
|
|
}
|
|
|
|
#[test]
|
|
#[should_panic(expected = "sigma must be finite and positive")]
|
|
fn a_zero_sigma_is_rejected() {
|
|
let _ = History::builder().sigma(0.0);
|
|
}
|
|
|
|
#[test]
|
|
#[should_panic(expected = "sigma must be finite and positive")]
|
|
fn a_negative_sigma_is_rejected() {
|
|
let _ = History::builder().sigma(-8.33);
|
|
}
|
|
|
|
#[test]
|
|
#[should_panic(expected = "sigma must be finite and positive")]
|
|
fn an_infinite_sigma_is_rejected() {
|
|
let _ = History::builder().sigma(f64::INFINITY);
|
|
}
|
|
|
|
#[test]
|
|
#[should_panic(expected = "beta must be finite and non-negative")]
|
|
fn a_negative_beta_is_rejected() {
|
|
let _ = History::builder().beta(-4.17);
|
|
}
|
|
|
|
#[test]
|
|
#[should_panic(expected = "beta must be finite and non-negative")]
|
|
fn a_non_finite_beta_is_rejected() {
|
|
let _ = History::builder().beta(f64::NAN);
|
|
}
|
|
|
|
/// Zero beta is deliberately allowed: performance is then exactly skill.
|
|
/// It has to reach a different fit than a positive beta, or "allowed"
|
|
/// would just mean "not checked".
|
|
#[test]
|
|
fn a_zero_beta_is_allowed_and_changes_the_fit() {
|
|
let fit = |beta: f64| {
|
|
let mut h = History::builder()
|
|
.mu(25.0)
|
|
.sigma(25.0 / 3.0)
|
|
.beta(beta)
|
|
.build();
|
|
h.record_winner(&"a", &"b", 1).unwrap();
|
|
let _ = h.converge().unwrap();
|
|
h.current_skill(&"a").unwrap()
|
|
};
|
|
let zero = fit(0.0);
|
|
let positive = fit(25.0 / 6.0);
|
|
assert!(zero.pi().is_finite() && zero.pi() > 0.0);
|
|
assert!(
|
|
(zero.pi() - positive.pi()).abs() > 1e-6,
|
|
"zero beta must not merely be ignored: {zero:?} vs {positive:?}"
|
|
);
|
|
}
|
|
}
|
|
|
|
/// 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::<i64, ConstantDrift>::new(Gaussian::default(), -4.17, ConstantDrift::new(0.0));
|
|
}
|
|
|
|
#[test]
|
|
#[should_panic(expected = "beta must be finite and non-negative")]
|
|
fn a_nan_beta_is_rejected_by_rating_new() {
|
|
let _ = Rating::<i64, ConstantDrift>::new(
|
|
Gaussian::default(),
|
|
f64::NAN,
|
|
ConstantDrift::new(0.0),
|
|
);
|
|
}
|
|
|
|
#[test]
|
|
fn a_zero_beta_is_accepted_by_rating_new() {
|
|
let _ =
|
|
Rating::<i64, ConstantDrift>::new(Gaussian::default(), 0.0, ConstantDrift::new(0.0));
|
|
}
|
|
|
|
/// `ConstantDrift` rejects at construction now that its field is private.
|
|
#[test]
|
|
#[should_panic(expected = "gamma must be finite and non-negative")]
|
|
fn a_negative_gamma_is_rejected_by_constant_drift_new() {
|
|
let _ = ConstantDrift::new(-0.0833);
|
|
}
|
|
|
|
#[test]
|
|
#[should_panic(expected = "gamma must be finite and non-negative")]
|
|
fn a_non_finite_gamma_is_rejected_by_constant_drift_new() {
|
|
let _ = ConstantDrift::new(f64::NAN);
|
|
}
|
|
|
|
#[test]
|
|
fn gamma_reads_back_what_was_given() {
|
|
assert_eq!(ConstantDrift::new(0.25).gamma(), 0.25);
|
|
assert_eq!(ConstantDrift::new(0.0).gamma(), 0.0);
|
|
}
|
|
|
|
/// `HistoryBuilder::drift` is generic and cannot inspect an arbitrary
|
|
/// `Drift`, so the check on the variance each competitor accumulates is
|
|
/// still needed — it is the only thing standing between a custom
|
|
/// implementation and a NaN fit. `ConstantDrift` can no longer reach it,
|
|
/// so this uses an implementation that can.
|
|
#[test]
|
|
fn a_custom_drift_returning_a_bad_variance_is_rejected_at_convergence() {
|
|
#[derive(Clone, Copy, Debug)]
|
|
struct BadDrift(f64);
|
|
|
|
impl trueskill_tt::Drift<i64> for BadDrift {
|
|
fn variance_delta(&self, _from: &i64, _to: &i64) -> f64 {
|
|
self.0
|
|
}
|
|
fn variance_for_elapsed(&self, _elapsed: i64) -> f64 {
|
|
self.0
|
|
}
|
|
}
|
|
|
|
for bad in [f64::NAN, f64::INFINITY, -1.0] {
|
|
let mut h = History::builder().drift(BadDrift(bad)).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",
|
|
..
|
|
}
|
|
),
|
|
"drift {bad}: {err:?}"
|
|
);
|
|
}
|
|
}
|
|
|
|
/// An ordinary drift is untouched.
|
|
#[test]
|
|
fn an_ordinary_drift_still_converges() {
|
|
let mut h = History::builder()
|
|
.drift(ConstantDrift::new(25.0 / 300.0))
|
|
.build();
|
|
h.record_winner(&"a", &"b", 1).unwrap();
|
|
h.record_winner(&"a", &"b", 5).unwrap();
|
|
assert!(h.converge().unwrap().converged);
|
|
}
|
|
}
|