feat!: validate mu, sigma and beta on HistoryBuilder
The last three unvalidated setters, beside `p_draw`, `score_sigma` and `convergence`, which all assert eagerly. Measured before choosing bounds: beta = 0 -> works, pi 0.0211 (vs 0.0193 at the default) beta = -4.17 -> bit-identical to +4.17 sigma = -8.33 -> bit-identical to +8.33 sigma = 0 -> NonFiniteResult, current_skill returns tau: NaN sigma = inf -> same mu = NaN -> same So the bounds are not the obvious ones. `beta = 0` is legitimate and meaningful — performance is then exactly skill, and the fit moves measurably rather than degenerating — so zero is allowed and a test pins that it reaches a different answer, since "allowed" would otherwise be indistinguishable from "unchecked". The negative cases are the quiet ones. `sigma` and `beta` enter inference only as squares, so a negative value behaves as its absolute value and the sign is dropped without comment. That is the same defect `Member::with_drift_scale` already rejects, for the reason already written there. The non-finite cases are detected today — `converge` reports NonFiniteResult — but a caller who reads `current_skill` first is handed `tau: NaN`, so rejecting at the boundary is what actually closes it. BREAKING CHANGE: `HistoryBuilder::mu`, `sigma` and `beta` now panic on values they previously accepted, matching the existing behaviour of `p_draw`, `score_sigma` and `convergence`. Refs #18 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011hcFjNDmHXZF8URGLku5zZ
This commit is contained in:
@@ -39,17 +39,55 @@ pub struct HistoryBuilder<
|
||||
}
|
||||
|
||||
impl<T: Time, D: Drift<T>, O: Observer<T>, K: Eq + Hash + Clone> HistoryBuilder<T, D, O, K> {
|
||||
/// Prior mean skill.
|
||||
///
|
||||
/// # Panics
|
||||
///
|
||||
/// Panics if `mu` is not finite. A non-finite prior mean poisons every
|
||||
/// posterior derived from it: `converge` reports `NonFiniteResult`, but a
|
||||
/// caller who reads `current_skill` first is handed `tau: NaN`.
|
||||
pub fn mu(mut self, mu: f64) -> Self {
|
||||
assert!(mu.is_finite(), "mu must be finite (got {mu})");
|
||||
self.mu = mu;
|
||||
self
|
||||
}
|
||||
|
||||
/// Prior standard deviation.
|
||||
///
|
||||
/// # Panics
|
||||
///
|
||||
/// Panics unless `sigma` is finite and strictly positive.
|
||||
///
|
||||
/// Zero and infinity both give a prior precision that is not a number, and
|
||||
/// the whole fit comes back NaN. A *negative* sigma is the quieter half:
|
||||
/// it is only ever squared, so `-8.33` produces bit-identical results to
|
||||
/// `8.33` — a sign the caller cannot have meant, silently ignored.
|
||||
pub fn sigma(mut self, sigma: f64) -> Self {
|
||||
assert!(
|
||||
sigma.is_finite() && sigma > 0.0,
|
||||
"sigma must be finite and positive (got {sigma})"
|
||||
);
|
||||
self.sigma = sigma;
|
||||
self
|
||||
}
|
||||
|
||||
/// Per-event performance noise.
|
||||
///
|
||||
/// # Panics
|
||||
///
|
||||
/// Panics unless `beta` is finite and non-negative.
|
||||
///
|
||||
/// 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 for the same reason as a negative
|
||||
/// `sigma` or `Member::with_drift_scale`: `beta` enters only as `beta^2`,
|
||||
/// so a negative value behaves as its absolute value and the sign is lost
|
||||
/// without comment.
|
||||
pub fn beta(mut self, beta: f64) -> Self {
|
||||
assert!(
|
||||
beta.is_finite() && beta >= 0.0,
|
||||
"beta must be finite and non-negative (got {beta})"
|
||||
);
|
||||
self.beta = beta;
|
||||
self
|
||||
}
|
||||
|
||||
@@ -184,3 +184,74 @@ fn ingestion_rejects_weights_that_do_not_match_their_team() {
|
||||
"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:?}"
|
||||
);
|
||||
}
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user