fix!: validate the constructors below HistoryBuilder
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) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011hcFjNDmHXZF8URGLku5zZ
This commit is contained in:
@@ -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::<i64, ConstantDrift>::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::<i64, ConstantDrift>::new(Gaussian::default(), f64::NAN, ConstantDrift(0.0));
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn a_zero_beta_is_accepted_by_rating_new() {
|
||||
let _ = Rating::<i64, ConstantDrift>::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);
|
||||
}
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user