fix!: seal ConstantDrift's field so gamma can be validated
`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
This commit is contained in:
Claude Opus 5
@@ -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<dyn Fn(f64) -> 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<Case> = vec![
|
||||
(
|
||||
"Gaussian::from_ms(sigma)",
|
||||
Box::new(|v| {
|
||||
let _ = Gaussian::from_ms(25.0, v);
|
||||
false
|
||||
}),
|
||||
),
|
||||
(
|
||||
"Rating::new(beta)",
|
||||
Box::new(|v| {
|
||||
let _ = Rating::<i64, ConstantDrift>::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<Case> = vec![
|
||||
(
|
||||
"Rating::new(beta)",
|
||||
Box::new(|v| {
|
||||
let _ = Rating::<i64, ConstantDrift>::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::<i64, ConstantDrift>::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::<i64, ConstantDrift>::new(Gaussian::default(), 0.0, ConstantDrift::new(0.0));
|
||||
}
|
||||
Reference in New Issue
Block a user