fix: reject convergence options that silently disable inference
`Game::ranked` and `Game::scored` validated `p_draw` and `score_sigma`
but never `convergence`. `ConvergenceOptions` has public fields and
`GameOptions` carries one, so a caller could hand the engine a set that
`HistoryBuilder`'s eager asserts never saw. Past that, the only guard
was a `debug_assert!`, which is gone in the profile users ship.
An `alpha` of zero is the bad case, and it fails silently rather than
loudly. Measured in release before the fix:
likelihoods: [[Gaussian { pi: 0.0, tau: 0.0 }],
[Gaussian { pi: 0.0, tau: 0.0 }]]
Every EP update unapplied, every likelihood uninformative, inference
returning the priors it was given — and an `OwnedGame` that looks
entirely ordinary to the caller. `HistoryBuilder::convergence` already
documents exactly this hazard; the `Game` constructors just did not
share the check.
Adds `ConvergenceOptions::validate`, called by both constructors.
Rejects `alpha` outside `(0.0, 1.0]` and negative `epsilon`; NaN fails
both comparisons and is rejected too.
`tests/validation.rs` states the release-mode guarantee for the whole
public surface, not just this hole, and CI already runs the suite in
release. Probing the other conditions #18 lists found five of eight
already enforced — ties without a draw probability, per-event score
sigma, weight/team dimensions, draw-probability range, score-sigma
range — so this closes the remaining gap rather than the whole issue.
The engine keeps its `debug_assert!`s as invariant documentation.
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:
@@ -20,6 +20,37 @@ pub struct ConvergenceOptions {
|
|||||||
pub alpha: f64,
|
pub alpha: f64,
|
||||||
}
|
}
|
||||||
|
|
||||||
|
impl ConvergenceOptions {
|
||||||
|
/// Reject values that would make inference silently meaningless.
|
||||||
|
///
|
||||||
|
/// `HistoryBuilder::convergence` asserts these eagerly, but the fields are
|
||||||
|
/// public and `GameOptions` carries a `ConvergenceOptions` — so a caller
|
||||||
|
/// can hand `Game::ranked` a set the builder never saw. In release the
|
||||||
|
/// engine's `debug_assert!`s are gone, and an `alpha` of zero leaves every
|
||||||
|
/// EP update unapplied: inference returns the priors, with every likelihood
|
||||||
|
/// uninformative and nothing to indicate anything went wrong.
|
||||||
|
///
|
||||||
|
/// # Errors
|
||||||
|
///
|
||||||
|
/// `InvalidParameter` if `alpha` is outside `(0.0, 1.0]` or `epsilon` is
|
||||||
|
/// negative. NaN fails both comparisons and is rejected.
|
||||||
|
pub(crate) fn validate(&self) -> Result<(), crate::InferenceError> {
|
||||||
|
if !(self.alpha > 0.0 && self.alpha <= 1.0) {
|
||||||
|
return Err(crate::InferenceError::InvalidParameter {
|
||||||
|
name: "alpha",
|
||||||
|
value: self.alpha,
|
||||||
|
});
|
||||||
|
}
|
||||||
|
if self.epsilon.is_nan() || self.epsilon < 0.0 {
|
||||||
|
return Err(crate::InferenceError::InvalidParameter {
|
||||||
|
name: "epsilon",
|
||||||
|
value: self.epsilon,
|
||||||
|
});
|
||||||
|
}
|
||||||
|
Ok(())
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
impl Default for ConvergenceOptions {
|
impl Default for ConvergenceOptions {
|
||||||
fn default() -> Self {
|
fn default() -> Self {
|
||||||
Self {
|
Self {
|
||||||
|
|||||||
+7
-2
@@ -433,6 +433,9 @@ impl<'a, T: Time, D: Drift<T>> Game<'a, T, D> {
|
|||||||
impl<T: Time, D: Drift<T>> Game<'_, T, D> {
|
impl<T: Time, D: Drift<T>> Game<'_, T, D> {
|
||||||
/// # Errors
|
/// # Errors
|
||||||
///
|
///
|
||||||
|
/// - `InvalidParameter` if `options.convergence` is out of range — an
|
||||||
|
/// `alpha` of zero would leave every EP update unapplied and silently
|
||||||
|
/// return the priors.
|
||||||
/// - `InvalidProbability` if `options.p_draw` is outside `[0.0, 1.0)`.
|
/// - `InvalidProbability` if `options.p_draw` is outside `[0.0, 1.0)`.
|
||||||
/// - `MismatchedShape` if the outcome's rank count differs from `teams.len()`.
|
/// - `MismatchedShape` if the outcome's rank count differs from `teams.len()`.
|
||||||
/// - `WrongOutcomeKind` if `outcome` is not `Outcome::Ranked`.
|
/// - `WrongOutcomeKind` if `outcome` is not `Outcome::Ranked`.
|
||||||
@@ -444,6 +447,7 @@ impl<T: Time, D: Drift<T>> Game<'_, T, D> {
|
|||||||
outcome: crate::Outcome,
|
outcome: crate::Outcome,
|
||||||
options: &GameOptions,
|
options: &GameOptions,
|
||||||
) -> Result<OwnedGame<T, D>, crate::InferenceError> {
|
) -> Result<OwnedGame<T, D>, crate::InferenceError> {
|
||||||
|
options.convergence.validate()?;
|
||||||
if !(0.0..1.0).contains(&options.p_draw) {
|
if !(0.0..1.0).contains(&options.p_draw) {
|
||||||
return Err(crate::InferenceError::InvalidProbability {
|
return Err(crate::InferenceError::InvalidProbability {
|
||||||
value: options.p_draw,
|
value: options.p_draw,
|
||||||
@@ -491,8 +495,8 @@ impl<T: Time, D: Drift<T>> Game<'_, T, D> {
|
|||||||
|
|
||||||
/// # Errors
|
/// # Errors
|
||||||
///
|
///
|
||||||
/// - `InvalidParameter` if `options.score_sigma` is not strictly positive,
|
/// - `InvalidParameter` if `options.score_sigma` is not strictly positive
|
||||||
/// or is NaN.
|
/// or is NaN, or if `options.convergence` is out of range.
|
||||||
/// - `MismatchedShape` if the outcome's score count differs from `teams.len()`.
|
/// - `MismatchedShape` if the outcome's score count differs from `teams.len()`.
|
||||||
/// - `WrongOutcomeKind` if `outcome` is not `Outcome::Scored`.
|
/// - `WrongOutcomeKind` if `outcome` is not `Outcome::Scored`.
|
||||||
pub fn scored(
|
pub fn scored(
|
||||||
@@ -500,6 +504,7 @@ impl<T: Time, D: Drift<T>> Game<'_, T, D> {
|
|||||||
outcome: crate::Outcome,
|
outcome: crate::Outcome,
|
||||||
options: &GameOptions,
|
options: &GameOptions,
|
||||||
) -> Result<OwnedGame<T, D>, crate::InferenceError> {
|
) -> Result<OwnedGame<T, D>, crate::InferenceError> {
|
||||||
|
options.convergence.validate()?;
|
||||||
if options.score_sigma <= 0.0 || options.score_sigma.is_nan() {
|
if options.score_sigma <= 0.0 || options.score_sigma.is_nan() {
|
||||||
return Err(crate::InferenceError::InvalidParameter {
|
return Err(crate::InferenceError::InvalidParameter {
|
||||||
name: "score_sigma",
|
name: "score_sigma",
|
||||||
|
|||||||
@@ -0,0 +1,186 @@
|
|||||||
|
//! 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(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:?}"
|
||||||
|
);
|
||||||
|
}
|
||||||
Reference in New Issue
Block a user