diff --git a/README.md b/README.md index fdf3c67..6f75eca 100644 --- a/README.md +++ b/README.md @@ -45,7 +45,7 @@ grows proportionally to time: variance_delta = elapsed * γ² ``` -This is the standard TrueSkill Through Time model. Pass a `ConstantDrift(gamma)` +This is the standard TrueSkill Through Time model. Pass a `ConstantDrift::new(gamma)` when constructing a `Rating`: ```rust @@ -53,9 +53,9 @@ use trueskill_tt::{ConstantDrift, Gaussian, Rating}; // gamma = 0.1 means skill can shift ~0.1 per time unit. let rating: Rating = - Rating::new(Gaussian::from_ms(0.0, 6.0), 1.0, ConstantDrift(0.1)); + Rating::new(Gaussian::from_ms(0.0, 6.0), 1.0, ConstantDrift::new(0.1)); -assert_eq!(rating.drift().0, 0.1); +assert_eq!(rating.drift().gamma(), 0.1); ``` The type annotation is load-bearing: `ConstantDrift` implements `Drift` for @@ -98,14 +98,14 @@ assert_eq!(history.log_evidence(), 0.0); ``` `HistoryBuilder::drift` is the only way to set a history's drift model; there is -no `gamma()` shorthand. The default is `ConstantDrift(GAMMA)`. +no `gamma()` shorthand. The default is `ConstantDrift::new(GAMMA)`. ### Per-competitor drift A `History` has one drift model, but individual competitors can scale it. `Member::with_drift_scale(s)` multiplies the drift *variance* that competitor -accumulates, so `s` is in the same units as `gamma`: `ConstantDrift(g)` at -scale `s` behaves exactly as `ConstantDrift(g * s)` would, for that competitor +accumulates, so `s` is in the same units as `gamma`: `ConstantDrift::new(g)` at +scale `s` behaves exactly as `ConstantDrift::new(g * s)` would, for that competitor alone. `0.0` pins a competitor still. That is what makes a **fixed reference point** @@ -115,7 +115,7 @@ strength, a rating floor, a course difficulty: ```rust use trueskill_tt::{ConstantDrift, Event, History, Member, Outcome, Team}; -let mut h = History::builder().drift(ConstantDrift(0.1)).build(); +let mut h = History::builder().drift(ConstantDrift::new(0.1)).build(); h.add_events(vec![Event { time: 0, diff --git a/benches/batch.rs b/benches/batch.rs index 2f30794..20bd6c7 100644 --- a/benches/batch.rs +++ b/benches/batch.rs @@ -17,7 +17,11 @@ fn criterion_benchmark(criterion: &mut Criterion) { agents.insert( agent, Competitor { - rating: Rating::new(Gaussian::from_ms(MU, SIGMA), BETA, ConstantDrift(GAMMA)), + rating: Rating::new( + Gaussian::from_ms(MU, SIGMA), + BETA, + ConstantDrift::new(GAMMA), + ), ..Default::default() }, ); diff --git a/benches/history_converge.rs b/benches/history_converge.rs index aff9a0c..f623c37 100644 --- a/benches/history_converge.rs +++ b/benches/history_converge.rs @@ -47,7 +47,7 @@ fn build_history_1v1( .mu(25.0) .sigma(25.0 / 3.0) .beta(25.0 / 6.0) - .drift(ConstantDrift(25.0 / 300.0)) + .drift(ConstantDrift::new(25.0 / 300.0)) .convergence(ConvergenceOptions { max_iter: 30, epsilon: 1e-6, diff --git a/benches/joint.rs b/benches/joint.rs index eb5aff1..e611979 100644 --- a/benches/joint.rs +++ b/benches/joint.rs @@ -16,7 +16,7 @@ fn fitted() -> History { .sigma(6.0) .beta(1.0) .score_sigma(2.0) - .drift(ConstantDrift(0.05)) + .drift(ConstantDrift::new(0.05)) .convergence(ConvergenceOptions { max_iter: 30, epsilon: 1e-10, diff --git a/benches/scored.rs b/benches/scored.rs index 6a3909d..1ee167f 100644 --- a/benches/scored.rs +++ b/benches/scored.rs @@ -9,7 +9,7 @@ fn bench_scored_history(c: &mut Criterion) { .mu(25.0) .sigma(25.0 / 3.0) .beta(25.0 / 6.0) - .drift(ConstantDrift(0.03)) + .drift(ConstantDrift::new(0.03)) .score_sigma(2.0) .build(); diff --git a/examples/atp.rs b/examples/atp.rs index 2a967d8..e7bd5a8 100644 --- a/examples/atp.rs +++ b/examples/atp.rs @@ -44,7 +44,7 @@ fn main() { let mut hist: History = History::builder_with_key() .sigma(1.6) - .drift(ConstantDrift(0.036)) + .drift(ConstantDrift::new(0.036)) .convergence(trueskill_tt::ConvergenceOptions { // This history needs 30 sweeps to reach the epsilon below. It was // capped at 10 until the `#[must_use]` on `ConvergenceReport` diff --git a/examples/scored.rs b/examples/scored.rs index 5a63f81..3566306 100644 --- a/examples/scored.rs +++ b/examples/scored.rs @@ -14,7 +14,7 @@ fn main() { .mu(25.0) .sigma(25.0 / 3.0) .beta(25.0 / 6.0) - .drift(ConstantDrift(0.03)) + .drift(ConstantDrift::new(0.03)) .score_sigma(2.0) // tune to data; smaller = trust margins more .build(); diff --git a/src/acquisition.rs b/src/acquisition.rs index 76a236f..23d1eea 100644 --- a/src/acquisition.rs +++ b/src/acquisition.rs @@ -208,7 +208,11 @@ mod tests { type R = Rating; fn rating(mu: f64, sigma: f64) -> R { - R::new(Gaussian::from_ms(mu, sigma), BETA, ConstantDrift(GAMMA)) + R::new( + Gaussian::from_ms(mu, sigma), + BETA, + ConstantDrift::new(GAMMA), + ) } fn options(p_draw: f64) -> GameOptions { diff --git a/src/drift.rs b/src/drift.rs index 8f89415..ba36486 100644 --- a/src/drift.rs +++ b/src/drift.rs @@ -22,24 +22,57 @@ 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 +/// # Why the field is private /// -/// `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. +/// `gamma` enters only as `gamma * gamma`, so a negative value is 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. That is the same sign-absorption `HistoryBuilder::sigma`, +/// `HistoryBuilder::beta`, `Gaussian::from_ms` and `Rating::new` all reject. /// -/// 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. +/// It could not be checked while the field was a public tuple position, because +/// there was no constructor to intercept. Validating inside +/// `variance_for_elapsed` would have been worse: it runs inside 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 for a guard, where +/// rejecting NaN turned the `NonFiniteResult` reporting path into a crash. /// -/// A non-finite `gamma` is a different matter and **is** rejected: +/// So [`ConstantDrift::new`] is the only way in, and it checks. Read the value +/// back with [`ConstantDrift::gamma`]. +/// +/// A non-finite gamma is caught a second time regardless: /// `History::converge` validates the drift variance each competitor actually -/// accumulates, which also covers a custom [`Drift`] implementation, and -/// reports `InferenceError::InvalidParameter`. +/// accumulates, which also covers a custom [`Drift`] implementation. #[derive(Clone, Copy, Debug)] -pub struct ConstantDrift(pub f64); +pub struct ConstantDrift(f64); + +impl ConstantDrift { + /// Drift of `gamma` standard deviations per unit time. + /// + /// # Panics + /// + /// Panics unless `gamma` is finite and non-negative. + /// + /// The field is private and this is the only constructor precisely so that + /// there is somewhere to check. While it was a public tuple field there was + /// nothing to intercept, and a negative gamma was silently squared away — + /// see the type docs. + #[must_use] + pub fn new(gamma: f64) -> Self { + assert!( + gamma.is_finite() && gamma >= 0.0, + "gamma must be finite and non-negative (got {gamma}); it is only ever \ + squared, so a negative value would silently behave as its absolute value" + ); + Self(gamma) + } + + /// Standard deviations of drift accumulated per unit time. + #[must_use] + pub fn gamma(&self) -> f64 { + self.0 + } +} impl Drift for ConstantDrift { fn variance_delta(&self, from: &T, to: &T) -> f64 { diff --git a/src/event.rs b/src/event.rs index 341dd54..f547cc3 100644 --- a/src/event.rs +++ b/src/event.rs @@ -99,8 +99,8 @@ impl Member { /// Scale how fast this competitor drifts, relative to the history's drift. /// /// The scale multiplies the drift *variance*, so it is in the same units as - /// `gamma`: `ConstantDrift(g)` at `scale = s` behaves exactly as - /// `ConstantDrift(g * s)` would for this competitor alone. + /// `gamma`: `ConstantDrift::new(g)` at `scale = s` behaves exactly as + /// `ConstantDrift::new(g * s)` would for this competitor alone. /// /// `0.0` pins the competitor still — useful for a reference point that /// shares a scale with moving competitors but should not itself move: a bot diff --git a/src/game.rs b/src/game.rs index 0977a48..8c97fed 100644 --- a/src/game.rs +++ b/src/game.rs @@ -623,12 +623,12 @@ mod tests { let t_a = R::new( Gaussian::from_ms(25.0, 25.0 / 3.0), 25.0 / 6.0, - ConstantDrift(25.0 / 300.0), + ConstantDrift::new(25.0 / 300.0), ); let t_b = R::new( Gaussian::from_ms(25.0, 25.0 / 3.0), 25.0 / 6.0, - ConstantDrift(25.0 / 300.0), + ConstantDrift::new(25.0 / 300.0), ); let w = [vec![1.0], vec![1.0]]; @@ -651,12 +651,12 @@ mod tests { let t_a = R::new( Gaussian::from_ms(29.0, 1.0), 25.0 / 6.0, - ConstantDrift(GAMMA), + ConstantDrift::new(GAMMA), ); let t_b = R::new( Gaussian::from_ms(25.0, 25.0 / 3.0), 25.0 / 6.0, - ConstantDrift(GAMMA), + ConstantDrift::new(GAMMA), ); let w = [vec![1.0], vec![1.0]]; @@ -676,8 +676,16 @@ mod tests { assert_ulps_eq!(a, Gaussian::from_ms(28.896475, 0.996604), epsilon = 1e-6); assert_ulps_eq!(b, Gaussian::from_ms(32.189211, 6.062063), epsilon = 1e-6); - let t_a = R::new(Gaussian::from_ms(1.139, 0.531), 1.0, ConstantDrift(0.2125)); - let t_b = R::new(Gaussian::from_ms(15.568, 0.51), 1.0, ConstantDrift(0.2125)); + let t_a = R::new( + Gaussian::from_ms(1.139, 0.531), + 1.0, + ConstantDrift::new(0.2125), + ); + let t_b = R::new( + Gaussian::from_ms(15.568, 0.51), + 1.0, + ConstantDrift::new(0.2125), + ); let w = [vec![1.0], vec![1.0]]; let g = Game::ranked_with_arena( @@ -699,17 +707,17 @@ mod tests { vec![R::new( Gaussian::from_ms(25.0, 25.0 / 3.0), 25.0 / 6.0, - ConstantDrift(25.0 / 300.0), + ConstantDrift::new(25.0 / 300.0), )], vec![R::new( Gaussian::from_ms(25.0, 25.0 / 3.0), 25.0 / 6.0, - ConstantDrift(25.0 / 300.0), + ConstantDrift::new(25.0 / 300.0), )], vec![R::new( Gaussian::from_ms(25.0, 25.0 / 3.0), 25.0 / 6.0, - ConstantDrift(25.0 / 300.0), + ConstantDrift::new(25.0 / 300.0), )], ]; @@ -779,12 +787,12 @@ mod tests { let t_a = R::new( Gaussian::from_ms(25.0, 25.0 / 3.0), 25.0 / 6.0, - ConstantDrift(25.0 / 300.0), + ConstantDrift::new(25.0 / 300.0), ); let t_b = R::new( Gaussian::from_ms(25.0, 25.0 / 3.0), 25.0 / 6.0, - ConstantDrift(25.0 / 300.0), + ConstantDrift::new(25.0 / 300.0), ); let w = [vec![1.0], vec![1.0]]; @@ -811,12 +819,12 @@ mod tests { let t_a = R::new( Gaussian::from_ms(25.0, 3.0), 25.0 / 6.0, - ConstantDrift(25.0 / 300.0), + ConstantDrift::new(25.0 / 300.0), ); let t_b = R::new( Gaussian::from_ms(29.0, 2.0), 25.0 / 6.0, - ConstantDrift(25.0 / 300.0), + ConstantDrift::new(25.0 / 300.0), ); let w = [vec![1.0], vec![1.0]]; @@ -842,17 +850,17 @@ mod tests { let t_a = R::new( Gaussian::from_ms(25.0, 25.0 / 3.0), 25.0 / 6.0, - ConstantDrift(25.0 / 300.0), + ConstantDrift::new(25.0 / 300.0), ); let t_b = R::new( Gaussian::from_ms(25.0, 25.0 / 3.0), 25.0 / 6.0, - ConstantDrift(25.0 / 300.0), + ConstantDrift::new(25.0 / 300.0), ); let t_c = R::new( Gaussian::from_ms(25.0, 25.0 / 3.0), 25.0 / 6.0, - ConstantDrift(25.0 / 300.0), + ConstantDrift::new(25.0 / 300.0), ); let w = [vec![1.0], vec![1.0], vec![1.0]]; @@ -879,17 +887,17 @@ mod tests { let t_a = R::new( Gaussian::from_ms(25.0, 3.0), 25.0 / 6.0, - ConstantDrift(25.0 / 300.0), + ConstantDrift::new(25.0 / 300.0), ); let t_b = R::new( Gaussian::from_ms(25.0, 3.0), 25.0 / 6.0, - ConstantDrift(25.0 / 300.0), + ConstantDrift::new(25.0 / 300.0), ); let t_c = R::new( Gaussian::from_ms(29.0, 2.0), 25.0 / 6.0, - ConstantDrift(25.0 / 300.0), + ConstantDrift::new(25.0 / 300.0), ); let w = [vec![1.0], vec![1.0], vec![1.0]]; @@ -918,29 +926,29 @@ mod tests { R::new( Gaussian::from_ms(12.0, 3.0), 25.0 / 6.0, - ConstantDrift(25.0 / 300.0), + ConstantDrift::new(25.0 / 300.0), ), R::new( Gaussian::from_ms(18.0, 3.0), 25.0 / 6.0, - ConstantDrift(25.0 / 300.0), + ConstantDrift::new(25.0 / 300.0), ), ]; let t_b = vec![R::new( Gaussian::from_ms(30.0, 3.0), 25.0 / 6.0, - ConstantDrift(25.0 / 300.0), + ConstantDrift::new(25.0 / 300.0), )]; let t_c = vec![ R::new( Gaussian::from_ms(14.0, 3.0), 25.0 / 6.0, - ConstantDrift(25.0 / 300.0), + ConstantDrift::new(25.0 / 300.0), ), R::new( Gaussian::from_ms(16., 3.0), 25.0 / 6.0, - ConstantDrift(25.0 / 300.0), + ConstantDrift::new(25.0 / 300.0), ), ]; @@ -970,12 +978,12 @@ mod tests { let t_a = vec![R::new( Gaussian::from_ms(25.0, 25.0 / 3.0), 25.0 / 6.0, - ConstantDrift(0.0), + ConstantDrift::new(0.0), )]; let t_b = vec![R::new( Gaussian::from_ms(25.0, 25.0 / 3.0), 25.0 / 6.0, - ConstantDrift(0.0), + ConstantDrift::new(0.0), )]; let w = [w_a, w_b]; @@ -1053,8 +1061,16 @@ mod tests { let w_a = vec![1.0]; let w_b = vec![0.0]; - let t_a = vec![R::new(Gaussian::from_ms(2.0, 6.0), 1.0, ConstantDrift(0.0))]; - let t_b = vec![R::new(Gaussian::from_ms(2.0, 6.0), 1.0, ConstantDrift(0.0))]; + let t_a = vec![R::new( + Gaussian::from_ms(2.0, 6.0), + 1.0, + ConstantDrift::new(0.0), + )]; + let t_b = vec![R::new( + Gaussian::from_ms(2.0, 6.0), + 1.0, + ConstantDrift::new(0.0), + )]; let w = [w_a, w_b]; let g = Game::ranked_with_arena( @@ -1081,8 +1097,16 @@ mod tests { let w_a = vec![1.0]; let w_b = vec![-1.0]; - let t_a = vec![R::new(Gaussian::from_ms(2.0, 6.0), 1.0, ConstantDrift(0.0))]; - let t_b = vec![R::new(Gaussian::from_ms(2.0, 6.0), 1.0, ConstantDrift(0.0))]; + let t_a = vec![R::new( + Gaussian::from_ms(2.0, 6.0), + 1.0, + ConstantDrift::new(0.0), + )]; + let t_b = vec![R::new( + Gaussian::from_ms(2.0, 6.0), + 1.0, + ConstantDrift::new(0.0), + )]; let w = [w_a, w_b]; let g = Game::ranked_with_arena( @@ -1125,7 +1149,7 @@ mod tests { let prior = R::new( Gaussian::from_ms(25.0, 25.0 / 3.0), 25.0 / 6.0, - ConstantDrift(25.0 / 300.0), + ConstantDrift::new(25.0 / 300.0), ); let teams = vec![vec![prior], vec![prior]]; let result = vec![10.0, 0.0]; // a beat b by 10 @@ -1175,7 +1199,7 @@ mod tests { let prior = R::new( Gaussian::from_ms(25.0, 25.0 / 3.0), 25.0 / 6.0, - ConstantDrift(25.0 / 300.0), + ConstantDrift::new(25.0 / 300.0), ); let opts = GameOptions { score_sigma: 1.0, @@ -1191,7 +1215,7 @@ mod tests { let prior = R::new( Gaussian::from_ms(25.0, 25.0 / 3.0), 25.0 / 6.0, - ConstantDrift(25.0 / 300.0), + ConstantDrift::new(25.0 / 300.0), ); let err = Game::scored( &[&[prior], &[prior]], @@ -1210,7 +1234,7 @@ mod tests { let prior = R::new( Gaussian::from_ms(25.0, 25.0 / 3.0), 25.0 / 6.0, - ConstantDrift(25.0 / 300.0), + ConstantDrift::new(25.0 / 300.0), ); let opts = GameOptions { score_sigma: 0.0, @@ -1237,12 +1261,12 @@ mod tests { R::new( Gaussian::from_ms(25.0, 25.0 / 3.0), 25.0 / 6.0, - ConstantDrift(0.0), + ConstantDrift::new(0.0), ), R::new( Gaussian::from_ms(25.0, 25.0 / 3.0), 25.0 / 6.0, - ConstantDrift(0.0), + ConstantDrift::new(0.0), ), ]; let w_a = vec![0.4, 0.8]; @@ -1251,12 +1275,12 @@ mod tests { R::new( Gaussian::from_ms(25.0, 25.0 / 3.0), 25.0 / 6.0, - ConstantDrift(0.0), + ConstantDrift::new(0.0), ), R::new( Gaussian::from_ms(25.0, 25.0 / 3.0), 25.0 / 6.0, - ConstantDrift(0.0), + ConstantDrift::new(0.0), ), ]; let w_b = vec![0.9, 0.6]; @@ -1370,7 +1394,7 @@ mod tests { vec![R::new( Gaussian::from_ms(25.0, 25.0 / 3.0), 25.0 / 6.0, - ConstantDrift(0.0), + ConstantDrift::new(0.0), )], ], &[1.0, 0.0], diff --git a/src/history.rs b/src/history.rs index 5612cf5..bfcae49 100644 --- a/src/history.rs +++ b/src/history.rs @@ -139,8 +139,8 @@ impl, O: Observer, K: Eq + Hash + Clone> HistoryBuilder< /// Panics if `score_sigma` is not strictly positive. pub fn score_sigma(mut self, score_sigma: f64) -> Self { assert!( - score_sigma > 0.0, - "score_sigma must be positive (got {score_sigma})" + score_sigma.is_finite() && score_sigma > 0.0, + "score_sigma must be finite and positive (got {score_sigma})" ); self.score_sigma = score_sigma; self @@ -224,7 +224,7 @@ impl Default for HistoryBuilder mu: MU, sigma: SIGMA, beta: BETA, - drift: ConstantDrift(GAMMA), + drift: ConstantDrift::new(GAMMA), p_draw: P_DRAW, score_sigma: 1.0, convergence: ConvergenceOptions::default(), @@ -358,7 +358,7 @@ impl History { mu: MU, sigma: SIGMA, beta: BETA, - drift: ConstantDrift(GAMMA), + drift: ConstantDrift::new(GAMMA), p_draw: P_DRAW, score_sigma: 1.0, convergence: ConvergenceOptions::default(), @@ -1663,7 +1663,7 @@ impl, O: Observer, K: Eq + Hash + Clone> History> = vec![ @@ -2957,7 +2957,7 @@ mod tests { .mu(0.0) .sigma(2.0) .beta(1.0) - .drift(ConstantDrift(0.0)) + .drift(ConstantDrift::new(0.0)) .build(); let events = make_events_1v1( @@ -3054,7 +3054,7 @@ mod tests { .mu(0.0) .sigma(2.0) .beta(1.0) - .drift(ConstantDrift(0.0)) + .drift(ConstantDrift::new(0.0)) .build(); let events = make_events_1v1( @@ -3227,7 +3227,7 @@ mod tests { .mu(0.0) .sigma(2.0) .beta(1.0) - .drift(ConstantDrift(0.0)) + .drift(ConstantDrift::new(0.0)) .build(); let events = make_events_1v1( @@ -3330,7 +3330,7 @@ mod tests { .mu(0.0) .sigma(2.0) .beta(1.0) - .drift(ConstantDrift(0.0)) + .drift(ConstantDrift::new(0.0)) .build(); let events = make_events_1v1( @@ -3434,7 +3434,7 @@ mod tests { .mu(2.0) .sigma(6.0) .beta(1.0) - .drift(ConstantDrift(0.0)) + .drift(ConstantDrift::new(0.0)) .build(); // empty results in old API = team 0 wins: a wins event 1, b wins event 2 @@ -3501,7 +3501,7 @@ mod tests { .mu(0.0) .sigma(2.0) .beta(1.0) - .drift(ConstantDrift(0.0)) + .drift(ConstantDrift::new(0.0)) .convergence(ConvergenceOptions { max_iter: 30, epsilon: 1e-6, @@ -3528,7 +3528,7 @@ mod tests { } #[test] - #[should_panic(expected = "score_sigma must be positive")] + #[should_panic(expected = "score_sigma must be finite and positive")] fn history_builder_rejects_zero_score_sigma() { let _ = History::builder().score_sigma(0.0).build(); } diff --git a/src/rating.rs b/src/rating.rs index 5de6b5c..a58fabc 100644 --- a/src/rating.rs +++ b/src/rating.rs @@ -110,7 +110,7 @@ impl Default for Rating { Self { prior: Gaussian::default(), beta: BETA, - drift: ConstantDrift(GAMMA), + drift: ConstantDrift::new(GAMMA), drift_scale: 1.0, _time: PhantomData, } diff --git a/src/time_slice.rs b/src/time_slice.rs index 30b23be..b00fd11 100644 --- a/src/time_slice.rs +++ b/src/time_slice.rs @@ -911,7 +911,7 @@ mod tests { rating: Rating::new( Gaussian::from_ms(25.0, 25.0 / 3.0), 25.0 / 6.0, - ConstantDrift(25.0 / 300.0), + ConstantDrift::new(25.0 / 300.0), ), ..Default::default() }, @@ -988,7 +988,7 @@ mod tests { rating: Rating::new( Gaussian::from_ms(25.0, 25.0 / 3.0), 25.0 / 6.0, - ConstantDrift(25.0 / 300.0), + ConstantDrift::new(25.0 / 300.0), ), ..Default::default() }, @@ -1068,7 +1068,7 @@ mod tests { rating: Rating::new( Gaussian::from_ms(25.0, 25.0 / 3.0), 25.0 / 6.0, - ConstantDrift(25.0 / 300.0), + ConstantDrift::new(25.0 / 300.0), ), ..Default::default() }, @@ -1171,7 +1171,7 @@ mod tests { rating: Rating::new( Gaussian::from_ms(25.0, 25.0 / 3.0), 25.0 / 6.0, - ConstantDrift(25.0 / 300.0), + ConstantDrift::new(25.0 / 300.0), ), ..Default::default() }, diff --git a/tests/additive_model.rs b/tests/additive_model.rs index 9738d3c..fbe0c8e 100644 --- a/tests/additive_model.rs +++ b/tests/additive_model.rs @@ -34,7 +34,7 @@ fn additive_structure_makes_sums_wide_and_differences_tight() { .sigma(6.0) .beta(1.0) .score_sigma(2.0) - .drift(ConstantDrift(0.0)) + .drift(ConstantDrift::new(0.0)) .convergence(ConvergenceOptions { max_iter: 20_000, epsilon: 1e-12, diff --git a/tests/api_shape.rs b/tests/api_shape.rs index 6a56e0d..31656d5 100644 --- a/tests/api_shape.rs +++ b/tests/api_shape.rs @@ -11,7 +11,7 @@ fn add_events_bulk_via_iter() { .sigma(2.0) .beta(1.0) .p_draw(0.0) - .drift(ConstantDrift(0.0)) + .drift(ConstantDrift::new(0.0)) .convergence(ConvergenceOptions { max_iter: 30, epsilon: 1e-6, @@ -53,7 +53,7 @@ fn add_events_draw() { .sigma(25.0 / 3.0) .beta(25.0 / 6.0) .p_draw(0.25) - .drift(ConstantDrift(25.0 / 300.0)) + .drift(ConstantDrift::new(25.0 / 300.0)) .build(); let events: Vec> = vec![Event { @@ -181,7 +181,7 @@ fn log_evidence_total_vs_subset() { .sigma(6.0) .beta(1.0) .p_draw(0.0) - .drift(ConstantDrift(0.0)) + .drift(ConstantDrift::new(0.0)) .build(); h.record_winner(&"a", &"b", 1).unwrap(); h.record_winner(&"b", &"a", 2).unwrap(); @@ -236,7 +236,7 @@ fn fluent_event_builder_scores() { .mu(25.0) .sigma(25.0 / 3.0) .beta(25.0 / 6.0) - .drift(ConstantDrift(0.0)) + .drift(ConstantDrift::new(0.0)) .build(); h.event(1) diff --git a/tests/constructor_validation.rs b/tests/constructor_validation.rs new file mode 100644 index 0000000..9a0574e --- /dev/null +++ b/tests/constructor_validation.rs @@ -0,0 +1,232 @@ +//! Every public entry point that takes a magnitude, in one place. +//! +//! 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` first, then `Game`'s own entry points, then the +//! constructors beneath both. A per-site fix cannot notice the site nobody +//! thought of. +//! +//! So this enumerates them. `sigma`, `beta` and `gamma` all enter inference +//! only as squares, which means a negative value does not fail — it behaves as +//! its absolute value, bit for bit, and the sign vanishes with no diagnostic. +//! Non-finite values poison every posterior derived from them. +//! +//! Adding a public constructor that takes one of these and not adding it here +//! is the failure this file exists to make harder. + +use std::panic::{AssertUnwindSafe, catch_unwind}; + +use trueskill_tt::{ConstantDrift, Gaussian, History, Member, Outcome, Rating}; + +/// Did the entry point refuse the value, by panic or by `Err`? +fn refuses(f: impl FnOnce() -> bool) -> bool { + catch_unwind(AssertUnwindSafe(f)).unwrap_or(true) +} + +/// One entry point, as a name and a closure that applies a value to it. +type Case = (&'static str, Box bool>); + +/// Entry points that must reject a negative magnitude. +/// +/// Each closure returns `true` if it refused by returning an error; a panic is +/// also a refusal and is caught. +#[test] +fn every_magnitude_parameter_rejects_a_negative_value() { + let cases: Vec = vec![ + ( + "Gaussian::from_ms(sigma)", + Box::new(|v| { + let _ = Gaussian::from_ms(25.0, v); + false + }), + ), + ( + "Rating::new(beta)", + Box::new(|v| { + let _ = Rating::::new( + Gaussian::default(), + v, + ConstantDrift::new(0.0), + ); + false + }), + ), + ( + "ConstantDrift::new(gamma)", + Box::new(|v| { + let _ = ConstantDrift::new(v); + false + }), + ), + ( + "HistoryBuilder::sigma", + Box::new(|v| { + let _ = History::builder().sigma(v); + false + }), + ), + ( + "HistoryBuilder::beta", + Box::new(|v| { + let _ = History::builder().beta(v); + false + }), + ), + ( + "HistoryBuilder::score_sigma", + Box::new(|v| { + let _ = History::builder().score_sigma(v); + false + }), + ), + ( + "HistoryBuilder::p_draw", + Box::new(|v| { + let _ = History::builder().p_draw(v); + false + }), + ), + ( + "Member::with_drift_scale (at ingestion)", + Box::new(|v| { + let mut h = History::builder().build(); + h.add_events(vec![trueskill_tt::Event { + time: 1i64, + teams: smallvec::smallvec![ + trueskill_tt::Team::with_members([Member::new("a").with_drift_scale(v)]), + trueskill_tt::Team::with_members([Member::new("b")]), + ], + outcome: Outcome::winner(0, 2), + }]) + .is_err() + }), + ), + ( + "Outcome::scores_with_sigma (at ingestion)", + Box::new(|v| { + let mut h = History::builder().build(); + h.add_events(vec![trueskill_tt::Event { + time: 1i64, + teams: smallvec::smallvec![ + trueskill_tt::Team::with_members([Member::new("a")]), + trueskill_tt::Team::with_members([Member::new("b")]), + ], + outcome: Outcome::scores_with_sigma([3.0, 1.0], v), + }]) + .is_err() + }), + ), + ]; + + let mut accepted = Vec::new(); + for (name, f) in &cases { + if !refuses(|| f(-1.0)) { + accepted.push(*name); + } + } + + assert!( + accepted.is_empty(), + "these accepted a negative magnitude, which is squared away silently \ + rather than honoured or refused:\n {}", + accepted.join("\n ") + ); +} + +/// Same set, for NaN and infinity. +/// +/// `Gaussian::from_ms` is deliberately absent: a broken fit produces a NaN +/// sigma legitimately and `converge` reports it as `NonFiniteResult`. Rejecting +/// it in the constructor turned that reporting path into a panic inside +/// inference — see the comment on `from_ms`. +#[test] +fn every_magnitude_parameter_rejects_a_non_finite_value() { + let cases: Vec = vec![ + ( + "Rating::new(beta)", + Box::new(|v| { + let _ = Rating::::new( + Gaussian::default(), + v, + ConstantDrift::new(0.0), + ); + false + }), + ), + ( + "ConstantDrift::new(gamma)", + Box::new(|v| { + let _ = ConstantDrift::new(v); + false + }), + ), + ( + "HistoryBuilder::sigma", + Box::new(|v| { + let _ = History::builder().sigma(v); + false + }), + ), + ( + "HistoryBuilder::beta", + Box::new(|v| { + let _ = History::builder().beta(v); + false + }), + ), + ( + "HistoryBuilder::mu", + Box::new(|v| { + let _ = History::builder().mu(v); + false + }), + ), + ( + "HistoryBuilder::score_sigma", + Box::new(|v| { + let _ = History::builder().score_sigma(v); + false + }), + ), + ( + "HistoryBuilder::p_draw", + Box::new(|v| { + let _ = History::builder().p_draw(v); + false + }), + ), + ]; + + let mut accepted = Vec::new(); + for (name, f) in &cases { + for bad in [f64::NAN, f64::INFINITY] { + if !refuses(|| f(bad)) { + accepted.push(format!("{name} accepted {bad}")); + } + } + } + + assert!( + accepted.is_empty(), + "these accepted a non-finite magnitude:\n {}", + accepted.join("\n ") + ); +} + +/// The suite must not pass by refusing everything. +#[test] +fn ordinary_values_are_still_accepted() { + let _ = Gaussian::from_ms(25.0, 8.33); + let _ = Rating::::new(Gaussian::default(), 4.17, ConstantDrift::new(0.05)); + let _ = ConstantDrift::new(0.0833); + let _ = History::builder() + .mu(25.0) + .sigma(8.33) + .beta(4.17) + .score_sigma(1.0) + .p_draw(0.1); + + // Zero beta and zero gamma are legitimate, not degenerate. + let _ = ConstantDrift::new(0.0); + let _ = Rating::::new(Gaussian::default(), 0.0, ConstantDrift::new(0.0)); +} diff --git a/tests/convergence_strictness.rs b/tests/convergence_strictness.rs index 7d6027c..fa18893 100644 --- a/tests/convergence_strictness.rs +++ b/tests/convergence_strictness.rs @@ -30,7 +30,7 @@ fn capped(max_iter: usize) -> H { .sigma(6.0) .beta(1.0) .score_sigma(2.0) - .drift(ConstantDrift(0.5)) + .drift(ConstantDrift::new(0.5)) .convergence(ConvergenceOptions { max_iter, epsilon: 1e-13, @@ -112,7 +112,7 @@ fn the_default_cap_clears_an_ordinary_history() { .sigma(6.0) .beta(1.0) .score_sigma(2.0) - .drift(ConstantDrift(0.05)) + .drift(ConstantDrift::new(0.05)) .build(); let mut events = Vec::new(); diff --git a/tests/cross_process_determinism.rs b/tests/cross_process_determinism.rs index da9b5ed..79cf320 100644 --- a/tests/cross_process_determinism.rs +++ b/tests/cross_process_determinism.rs @@ -32,7 +32,7 @@ fn fitted() -> H { .sigma(6.0) .beta(1.0) .score_sigma(2.0) - .drift(ConstantDrift(0.05)) + .drift(ConstantDrift::new(0.05)) .unknown_keys(UnknownKeys::Prior) .convergence(ConvergenceOptions { max_iter: 20_000, diff --git a/tests/degenerate_inputs.rs b/tests/degenerate_inputs.rs index 01ef560..61f3c94 100644 --- a/tests/degenerate_inputs.rs +++ b/tests/degenerate_inputs.rs @@ -17,7 +17,7 @@ fn rating() -> R { R::new( Gaussian::from_ms(25.0, 25.0 / 3.0), 25.0 / 6.0, - ConstantDrift(25.0 / 300.0), + ConstantDrift::new(25.0 / 300.0), ) } @@ -281,8 +281,16 @@ fn log_evidence_survives_a_long_diff_chain() { /// `erfc` approximation; the evidence floor keeps `ln` finite. #[test] fn log_evidence_finite_for_near_certain_outcome() { - let overwhelming = R::new(Gaussian::from_ms(5_000.0, 0.5), 1.0, ConstantDrift(0.0)); - let hopeless = R::new(Gaussian::from_ms(-5_000.0, 0.5), 1.0, ConstantDrift(0.0)); + let overwhelming = R::new( + Gaussian::from_ms(5_000.0, 0.5), + 1.0, + ConstantDrift::new(0.0), + ); + let hopeless = R::new( + Gaussian::from_ms(-5_000.0, 0.5), + 1.0, + ConstantDrift::new(0.0), + ); let a = [overwhelming]; let b = [hopeless]; let teams: Vec<&[R]> = vec![&a, &b]; diff --git a/tests/determinism.rs b/tests/determinism.rs index e6c1da5..bd6657c 100644 --- a/tests/determinism.rs +++ b/tests/determinism.rs @@ -43,7 +43,7 @@ fn build_and_converge() -> Fingerprint { .mu(25.0) .sigma(25.0 / 3.0) .beta(25.0 / 6.0) - .drift(ConstantDrift(25.0 / 300.0)) + .drift(ConstantDrift::new(25.0 / 300.0)) .convergence(ConvergenceOptions { max_iter: 20_000, epsilon: 1e-9, diff --git a/tests/drift_scale.rs b/tests/drift_scale.rs index 50d8904..310a687 100644 --- a/tests/drift_scale.rs +++ b/tests/drift_scale.rs @@ -2,7 +2,7 @@ //! //! The scale multiplies the *variance* the history's `Drift` contributes for //! that competitor, so `scale` is in the same units as `gamma`: -//! `ConstantDrift(g)` at `scale = s` behaves as `ConstantDrift(g * s)` would. +//! `ConstantDrift::new(g)` at `scale = s` behaves as `ConstantDrift::new(g * s)` would. //! `scale = 0.0` pins a competitor still — an anchor, a rating floor, a course //! difficulty — while everyone around them keeps drifting. @@ -53,7 +53,7 @@ fn fit(events: Vec>, gamma: f64) -> Fit { .sigma(25.0 / 3.0) .beta(25.0 / 6.0) .p_draw(0.0) - .drift(ConstantDrift(gamma)) + .drift(ConstantDrift::new(gamma)) .convergence(CONVERGENCE) .build(); @@ -160,7 +160,7 @@ fn scale_is_equivalent_to_scaling_gamma() { assert_eq!(t_l, t_r); assert!( (g_l.mu() - g_r.mu()).abs() < 1e-9 && (g_l.sigma() - g_r.sigma()).abs() < 1e-9, - "ConstantDrift(0.3) at scale 0.5 must equal ConstantDrift(0.15) for {key} at \ + "ConstantDrift::new(0.3) at scale 0.5 must equal ConstantDrift::new(0.15) for {key} at \ t={t_l}: ({}, {}) vs ({}, {})", g_l.mu(), g_l.sigma(), @@ -218,7 +218,7 @@ fn mixed_static_and_drifting_graph_converges() { .sigma(25.0 / 3.0) .beta(25.0 / 6.0) .p_draw(0.0) - .drift(ConstantDrift(25.0 / 300.0)) + .drift(ConstantDrift::new(25.0 / 300.0)) .convergence(CONVERGENCE) .build(); @@ -259,7 +259,7 @@ fn mixed_static_and_drifting_graph_converges() { fn reject(scale: f64) -> InferenceError { let mut h = History::builder() - .drift(ConstantDrift(25.0 / 300.0)) + .drift(ConstantDrift::new(25.0 / 300.0)) .build(); let events: Vec> = vec![Event { @@ -360,7 +360,7 @@ fn drift_scale_applies_when_set_after_first_appearance() { .sigma(25.0 / 3.0) .beta(25.0 / 6.0) .p_draw(0.0) - .drift(ConstantDrift(25.0 / 300.0)) + .drift(ConstantDrift::new(25.0 / 300.0)) .convergence(CONVERGENCE) .build(); diff --git a/tests/equivalence.rs b/tests/equivalence.rs index cabd076..46db26b 100644 --- a/tests/equivalence.rs +++ b/tests/equivalence.rs @@ -12,7 +12,11 @@ use trueskill_tt::{ConstantDrift, Game, GameOptions, Gaussian, Outcome, Rating}; type R = Rating; fn ts_rating(mu: f64, sigma: f64, beta: f64, gamma: f64) -> R { - R::new(Gaussian::from_ms(mu, sigma), beta, ConstantDrift(gamma)) + R::new( + Gaussian::from_ms(mu, sigma), + beta, + ConstantDrift::new(gamma), + ) } #[test] diff --git a/tests/event_builder_members.rs b/tests/event_builder_members.rs index eb5ccc8..f182ab0 100644 --- a/tests/event_builder_members.rs +++ b/tests/event_builder_members.rs @@ -19,7 +19,7 @@ fn history() -> H { .sigma(6.0) .beta(1.0) .score_sigma(2.0) - .drift(ConstantDrift(0.5)) + .drift(ConstantDrift::new(0.5)) .convergence(ConvergenceOptions { max_iter: 20_000, epsilon: 1e-13, diff --git a/tests/game.rs b/tests/game.rs index 8f53dac..5330523 100644 --- a/tests/game.rs +++ b/tests/game.rs @@ -8,7 +8,7 @@ fn default_rating() -> R { R::new( Gaussian::from_ms(25.0, 25.0 / 3.0), 25.0 / 6.0, - ConstantDrift(25.0 / 300.0), + ConstantDrift::new(25.0 / 300.0), ) } @@ -40,7 +40,7 @@ fn game_one_v_one_shortcut() { #[test] fn game_ranked_rejects_bad_p_draw() { - let a = R::new(Gaussian::default(), 1.0, ConstantDrift(0.0)); + let a = R::new(Gaussian::default(), 1.0, ConstantDrift::new(0.0)); let err = Game::::ranked( &[&[a], &[a]], Outcome::winner(0, 2), @@ -56,7 +56,7 @@ fn game_ranked_rejects_bad_p_draw() { #[test] fn game_ranked_rejects_mismatched_ranks() { - let a = R::new(Gaussian::default(), 1.0, ConstantDrift(0.0)); + let a = R::new(Gaussian::default(), 1.0, ConstantDrift::new(0.0)); let err = Game::::ranked( &[&[a], &[a]], Outcome::ranking([0, 1, 2]), diff --git a/tests/joint_handle.rs b/tests/joint_handle.rs index 9a72050..5dc5a99 100644 --- a/tests/joint_handle.rs +++ b/tests/joint_handle.rs @@ -41,7 +41,7 @@ fn history(unknown: UnknownKeys) -> H { .sigma(6.0) .beta(1.0) .score_sigma(2.0) - .drift(ConstantDrift(0.5)) + .drift(ConstantDrift::new(0.5)) .unknown_keys(unknown) .convergence(ConvergenceOptions { max_iter: 20_000, @@ -158,7 +158,7 @@ fn drift_free_competitors_shrink_the_joint_by_the_slice_count() { .sigma(6.0) .beta(1.0) .score_sigma(2.0) - .drift(ConstantDrift(gamma)) + .drift(ConstantDrift::new(gamma)) .convergence(ConvergenceOptions { max_iter: 20_000, epsilon: 1e-13, @@ -197,7 +197,7 @@ fn pinned_competitors_collapse_consecutive_appearances() { .sigma(6.0) .beta(1.0) .score_sigma(2.0) - .drift(ConstantDrift(0.0)) + .drift(ConstantDrift::new(0.0)) .convergence(ConvergenceOptions { max_iter: 20_000, epsilon: 1e-13, @@ -284,7 +284,7 @@ fn a_drift_too_small_to_represent_collapses_rather_than_corrupting() { .sigma(6.0) .beta(1.0) .score_sigma(2.0) - .drift(ConstantDrift(0.5)) + .drift(ConstantDrift::new(0.5)) .convergence(ConvergenceOptions { max_iter: 20_000, epsilon: 1e-13, diff --git a/tests/large_history_converges_finite.rs b/tests/large_history_converges_finite.rs index 30e6d24..50b376b 100644 --- a/tests/large_history_converges_finite.rs +++ b/tests/large_history_converges_finite.rs @@ -27,7 +27,7 @@ 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)) + .drift(ConstantDrift::new(0.1)) .convergence(ConvergenceOptions { max_iter: ITERATIONS, epsilon: EPSILON, diff --git a/tests/marginal_calibration.rs b/tests/marginal_calibration.rs index 7997bbd..bfe8672 100644 --- a/tests/marginal_calibration.rs +++ b/tests/marginal_calibration.rs @@ -140,7 +140,7 @@ fn fitted( .sigma(SIGMA0) .beta(BETA) .score_sigma(SCORE_SIGMA) - .drift(ConstantDrift(0.0)) + .drift(ConstantDrift::new(0.0)) .convergence(ConvergenceOptions { max_iter: 20_000, epsilon: 1e-13, @@ -324,7 +324,7 @@ fn cost_scaling() { let names: Vec = (0..n).map(|i| format!("c{i}")).collect(); let mut h: History = History::builder_with_key() .score_sigma(2.0) - .drift(ConstantDrift(0.0)) + .drift(ConstantDrift::new(0.0)) .convergence(ConvergenceOptions { max_iter: 200, epsilon: 1e-8, diff --git a/tests/non_finite_results.rs b/tests/non_finite_results.rs index b7db0f0..26f7424 100644 --- a/tests/non_finite_results.rs +++ b/tests/non_finite_results.rs @@ -191,7 +191,7 @@ fn a_narrow_draw_margin_far_into_the_tail_still_fits() { .sigma(sd) .beta(beta) .p_draw(p_draw) - .drift(ConstantDrift(0.0)) + .drift(ConstantDrift::new(0.0)) .build(); h.add_events(vec![Event { time: 1i64, diff --git a/tests/predict_margin.rs b/tests/predict_margin.rs index cd54fb7..95b70c5 100644 --- a/tests/predict_margin.rs +++ b/tests/predict_margin.rs @@ -14,7 +14,7 @@ fn builder( .sigma(6.0) .beta(1.0) .score_sigma(2.0) - .drift(ConstantDrift(0.0)) + .drift(ConstantDrift::new(0.0)) .unknown_keys(policy) .convergence(ConvergenceOptions { max_iter: 5_000, diff --git a/tests/prediction_bounds.rs b/tests/prediction_bounds.rs index eedde48..0b9267e 100644 --- a/tests/prediction_bounds.rs +++ b/tests/prediction_bounds.rs @@ -74,8 +74,16 @@ fn information_gain_never_exceeds_the_entropy_of_the_outcome() { let sigma_b = rng.log_uniform(1e-4, 1e2); let beta = rng.log_uniform(1e-4, 1e1); - let a = R::new(Gaussian::from_ms(mu_a, sigma_a), beta, ConstantDrift(0.0)); - let b = R::new(Gaussian::from_ms(mu_b, sigma_b), beta, ConstantDrift(0.0)); + let a = R::new( + Gaussian::from_ms(mu_a, sigma_a), + beta, + ConstantDrift::new(0.0), + ); + let b = R::new( + Gaussian::from_ms(mu_b, sigma_b), + beta, + ConstantDrift::new(0.0), + ); let options = GameOptions { p_draw: 0.0, ..GameOptions::default() @@ -129,12 +137,12 @@ fn the_known_ceiling_violation_no_longer_answers_wrongly() { let a = R::new( Gaussian::from_ms(9.577_887_112_129_012, 0.000_132_507_526_585_134_38), 0.000_307_235_559_013_096_2, - ConstantDrift(0.0), + ConstantDrift::new(0.0), ); let b = R::new( Gaussian::from_ms(-14.114_932_828_525_696, 91.586_690_140_921_16), 0.000_307_235_559_013_096_2, - ConstantDrift(0.0), + ConstantDrift::new(0.0), ); let options = GameOptions { p_draw: 0.0, diff --git a/tests/record_winner.rs b/tests/record_winner.rs index d040d62..2e2b84b 100644 --- a/tests/record_winner.rs +++ b/tests/record_winner.rs @@ -6,7 +6,7 @@ fn record_winner_builds_history() { .mu(25.0) .sigma(25.0 / 3.0) .beta(25.0 / 6.0) - .drift(ConstantDrift(25.0 / 300.0)) + .drift(ConstantDrift::new(25.0 / 300.0)) .convergence(ConvergenceOptions { max_iter: 30, epsilon: 1e-6, @@ -43,7 +43,7 @@ fn record_draw_with_p_draw_set() { .mu(25.0) .sigma(25.0 / 3.0) .beta(25.0 / 6.0) - .drift(ConstantDrift(25.0 / 300.0)) + .drift(ConstantDrift::new(25.0 / 300.0)) .p_draw(0.25) .build(); diff --git a/tests/registration.rs b/tests/registration.rs index df7dee4..45555ae 100644 --- a/tests/registration.rs +++ b/tests/registration.rs @@ -21,7 +21,7 @@ fn history() -> H { .sigma(6.0) .beta(1.0) .score_sigma(2.0) - .drift(ConstantDrift(0.5)) + .drift(ConstantDrift::new(0.5)) .convergence(ConvergenceOptions { max_iter: 20_000, epsilon: 1e-13, diff --git a/tests/scored.rs b/tests/scored.rs index a1419e3..8623817 100644 --- a/tests/scored.rs +++ b/tests/scored.rs @@ -9,7 +9,7 @@ fn scored_two_team_one_event_pulls_winner_up() { .mu(0.0) .sigma(2.0) .beta(1.0) - .drift(ConstantDrift(0.0)) + .drift(ConstantDrift::new(0.0)) .score_sigma(1.0) .build(); @@ -46,7 +46,7 @@ fn scored_zero_margin_treats_as_tie() { .mu(0.0) .sigma(2.0) .beta(1.0) - .drift(ConstantDrift(0.0)) + .drift(ConstantDrift::new(0.0)) .score_sigma(1.0) .build(); @@ -88,7 +88,7 @@ fn scored_three_team_partial_order() { .mu(0.0) .sigma(2.0) .beta(1.0) - .drift(ConstantDrift(0.0)) + .drift(ConstantDrift::new(0.0)) .score_sigma(1.0) .build(); diff --git a/tests/time_expanded_joint.rs b/tests/time_expanded_joint.rs index 1080a1a..5ace6c1 100644 --- a/tests/time_expanded_joint.rs +++ b/tests/time_expanded_joint.rs @@ -24,7 +24,7 @@ fn history(gamma: f64) -> H { .sigma(SIGMA0) .beta(BETA) .score_sigma(SCORE_SIGMA) - .drift(ConstantDrift(gamma)) + .drift(ConstantDrift::new(gamma)) .unknown_keys(UnknownKeys::Reject) .convergence(ConvergenceOptions { max_iter: 20_000, diff --git a/tests/validation.rs b/tests/validation.rs index a798fdf..cbb5abb 100644 --- a/tests/validation.rs +++ b/tests/validation.rs @@ -22,7 +22,7 @@ fn rating() -> R { R::new( Gaussian::from_ms(25.0, 25.0 / 3.0), 25.0 / 6.0, - ConstantDrift(0.0), + ConstantDrift::new(0.0), ) } @@ -286,33 +286,66 @@ mod constructor_parameters { #[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)); + let _ = + Rating::::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::::new(Gaussian::default(), f64::NAN, ConstantDrift(0.0)); + let _ = Rating::::new( + Gaussian::default(), + f64::NAN, + ConstantDrift::new(0.0), + ); } #[test] fn a_zero_beta_is_accepted_by_rating_new() { - let _ = Rating::::new(Gaussian::default(), 0.0, ConstantDrift(0.0)); + let _ = + Rating::::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 is on the variance each competitor actually - /// accumulates. That also covers a custom implementation. + /// `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_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(); + fn a_custom_drift_returning_a_bad_variance_is_rejected_at_convergence() { + #[derive(Clone, Copy, Debug)] + struct BadDrift(f64); + + impl trueskill_tt::Drift 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(); @@ -324,7 +357,7 @@ mod constructor_parameters { .. } ), - "gamma {gamma}: {err:?}" + "drift {bad}: {err:?}" ); } } @@ -333,7 +366,7 @@ mod constructor_parameters { #[test] fn an_ordinary_drift_still_converges() { let mut h = History::builder() - .drift(ConstantDrift(25.0 / 300.0)) + .drift(ConstantDrift::new(25.0 / 300.0)) .build(); h.record_winner(&"a", &"b", 1).unwrap(); h.record_winner(&"a", &"b", 5).unwrap(); diff --git a/tests/variance_reduction.rs b/tests/variance_reduction.rs index 0255061..c1abcea 100644 --- a/tests/variance_reduction.rs +++ b/tests/variance_reduction.rs @@ -35,7 +35,7 @@ fn fit(extra: Option>, policy: UnknownKeys) -> H { .sigma(6.0) .beta(1.0) .score_sigma(2.0) - .drift(ConstantDrift(0.0)) + .drift(ConstantDrift::new(0.0)) .unknown_keys(policy) .convergence(ConvergenceOptions { max_iter: 20_000,