diff --git a/README.md b/README.md index 1ab442d..6411954 100644 --- a/README.md +++ b/README.md @@ -134,10 +134,15 @@ h.add_events(vec![Event { h.converge().unwrap(); ``` -Like `with_prior`, the scale is **competitor configuration captured at first -appearance** — setting it on a key the history already knows has no effect. It -must be finite and non-negative; ingestion otherwise fails with -`InferenceError::InvalidParameter`. +Like `with_prior`, the scale is **competitor configuration, not a per-event +value**: it applies to the competitor for the whole history, and it applies +whenever it is supplied — including on a key the history already knows. +Configuring one late still refits the whole history rather than taking effect +only from that event onward, because `converge` refits from competitor state. +Repeating the same value is inert; supplying two *different* values for one +competitor within a single batch is `InferenceError::ConflictingCompetitorConfig`, +since events in a batch have no order. The scale must be finite and +non-negative; ingestion otherwise fails with `InferenceError::InvalidParameter`. Note that the fluent `EventBuilder` (`h.event(t).team([...])`) sets weights but not `drift_scale` or `prior`; those need the typed `Event` / `Team` / `Member` diff --git a/src/event.rs b/src/event.rs index d7ed014..341dd54 100644 --- a/src/event.rs +++ b/src/event.rs @@ -88,7 +88,9 @@ impl Member { /// Set this competitor's starting skill estimate. /// - /// Captured at the competitor's first appearance; see the type docs. + /// Competitor configuration, not a per-event value: it applies for the + /// whole history and applies whenever it is supplied, including on a key + /// the history already knows. See the type docs. pub fn with_prior(mut self, prior: Gaussian) -> Self { self.prior = Some(prior); self @@ -104,7 +106,8 @@ impl Member { /// shares a scale with moving competitors but should not itself move: a bot /// at a known strength, a rating floor, a course difficulty. /// - /// Captured at the competitor's first appearance; see the type docs. + /// Applies for the whole history and whenever it is supplied, including on + /// a key the history already knows; see the type docs. /// Must be finite and non-negative, or ingestion fails with /// [`InferenceError::InvalidParameter`](crate::InferenceError::InvalidParameter). pub fn with_drift_scale(mut self, scale: f64) -> Self { diff --git a/src/history.rs b/src/history.rs index 989138a..8046fad 100644 --- a/src/history.rs +++ b/src/history.rs @@ -1505,6 +1505,50 @@ impl, O: Observer, K: Eq + Hash + Clone> History "rank", + EventKind::Scored { .. } => "score", + }; + for value in event_results { + if !value.is_finite() { + return Err(InferenceError::InvalidParameter { + name, + value: *value, + }); + } + } + } + } + // Chokepoint for tie validation: every ingestion route lands here, // including `record_draw`, which builds its results directly rather // than going through `Outcome`. diff --git a/tests/degenerate_inputs.rs b/tests/degenerate_inputs.rs index 3c783b2..01ef560 100644 --- a/tests/degenerate_inputs.rs +++ b/tests/degenerate_inputs.rs @@ -170,8 +170,9 @@ fn event_builder_rejects_a_weights_length_mismatch() { fn event_builder_weights_mismatch_leaves_the_history_untouched() { let mut h = History::default(); - // Two teams, so ingestion would otherwise succeed — a one-team event is - // rejected for an unrelated reason and would pass this vacuously. + // Two teams, so ingestion would otherwise succeed. A one-team event is + // rejected as `NotEnoughTeams` before the weights are ever examined, so + // building this with one team would pass vacuously. let _ = h .event(1) .team(["a"]) diff --git a/tests/ingestion_shape.rs b/tests/ingestion_shape.rs new file mode 100644 index 0000000..7b1c36e --- /dev/null +++ b/tests/ingestion_shape.rs @@ -0,0 +1,147 @@ +//! Malformed events must be rejected at the ingestion boundary. +//! +//! Every case here was reachable from safe public API in a release build. Two +//! of them are the two shapes this crate's defects keep taking: a panic from +//! deep inside inference, and a finite, plausible-looking posterior computed +//! from an event that should never have been accepted. +//! +//! `InferenceError::NotEnoughTeams` and `EmptyTeam` already existed when these +//! were found — they were checked on the prediction paths and nowhere else, so +//! ingestion could still manufacture the states they describe. + +use smallvec::smallvec; +use trueskill_tt::{Event, History, InferenceError, Member, Outcome, Team}; + +type Ev = Event; + +fn history() -> History +{ + History::builder().score_sigma(1.0).build() +} + +fn teams(names: &[&[&'static str]]) -> smallvec::SmallVec<[Team<&'static str>; 4]> { + names + .iter() + .map(|team| Team::with_members(team.iter().map(|k| Member::new(*k)))) + .collect() +} + +/// The regression this file exists for: `run_chain` builds one diff link per +/// adjacent pair of teams, so a one-team event left it indexing `links[1..]` +/// on an empty vector and panicked — in release, from `History::add_events`. +#[test] +fn a_one_team_event_is_an_error_not_a_panic() { + let mut h = history(); + let err = h + .add_events(vec![Ev { + time: 1, + teams: teams(&[&["a"]]), + outcome: Outcome::winner(0, 1), + }]) + .unwrap_err(); + assert!( + matches!(err, InferenceError::NotEnoughTeams { got: 1 }), + "{err:?}" + ); +} + +#[test] +fn a_zero_team_event_is_an_error() { + let mut h = history(); + let err = h + .add_events(vec![Ev { + time: 1, + teams: smallvec![], + outcome: Outcome::ranking([]), + }]) + .unwrap_err(); + assert!( + matches!(err, InferenceError::NotEnoughTeams { got: 0 }), + "{err:?}" + ); +} + +/// The quiet half. An empty team contributes no performance, so before this +/// was rejected the event converged and handed back a finite posterior for its +/// opponent — a plausible constant computed from nothing. +#[test] +fn an_empty_team_is_an_error_rather_than_a_free_win() { + let mut h = history(); + let err = h + .add_events(vec![Ev { + time: 1, + teams: teams(&[&[], &["b"]]), + outcome: Outcome::winner(0, 2), + }]) + .unwrap_err(); + assert!( + matches!(err, InferenceError::EmptyTeam { team: 0 }), + "{err:?}" + ); + // Nothing was recorded, so the history is still empty. + assert!(h.current_skill(&"b").is_none()); +} + +#[test] +fn an_empty_team_is_reported_by_position() { + let mut h = history(); + let err = h + .add_events(vec![Ev { + time: 1, + teams: teams(&[&["a"], &[]]), + outcome: Outcome::winner(0, 2), + }]) + .unwrap_err(); + assert!( + matches!(err, InferenceError::EmptyTeam { team: 1 }), + "{err:?}" + ); +} + +/// A NaN score used to ingest cleanly. `converge` reported `NonFiniteResult`, +/// but a caller who read `current_skill` first was handed `tau: NaN` with +/// nothing to say so. +#[test] +fn a_non_finite_score_is_rejected_at_ingestion() { + for bad in [f64::NAN, f64::INFINITY, f64::NEG_INFINITY] { + let mut h = history(); + let err = h + .add_events(vec![Ev { + time: 1, + teams: teams(&[&["a"], &["b"]]), + outcome: Outcome::scores([bad, 0.0]), + }]) + .unwrap_err(); + assert!( + matches!(err, InferenceError::InvalidParameter { name: "score", .. }), + "{bad}: {err:?}" + ); + assert!(h.current_skill(&"a").is_none(), "{bad} was recorded anyway"); + } +} + +/// The fluent builder routes through the same chokepoint, so it inherits the +/// checks rather than needing its own. +#[test] +fn the_event_builder_inherits_the_shape_checks() { + let mut h = history(); + let err = h.event(1).team(["a"]).winner(0).commit().unwrap_err(); + assert!( + matches!(err, InferenceError::NotEnoughTeams { got: 1 }), + "{err:?}" + ); +} + +/// A well-formed event is untouched by any of this. +#[test] +fn a_well_formed_event_still_ingests() { + let mut h = history(); + h.add_events(vec![Ev { + time: 1, + teams: teams(&[&["a"], &["b"]]), + outcome: Outcome::scores([3.0, 1.0]), + }]) + .unwrap(); + assert!(h.converge().unwrap().converged); + assert!(h.current_skill(&"a").unwrap().mu() > h.current_skill(&"b").unwrap().mu()); +}