diff --git a/src/convergence.rs b/src/convergence.rs index 10309f8..e7041fd 100644 --- a/src/convergence.rs +++ b/src/convergence.rs @@ -62,10 +62,17 @@ impl Default for ConvergenceOptions { } /// Post-hoc summary of a `History::converge` call. +/// +/// From [`History::converge`](crate::History::converge) this always describes a +/// converged fit — stopping at `max_iter` is +/// [`InferenceError::NotConverged`](crate::InferenceError::NotConverged) there. +/// From [`History::converge_partial`](crate::History::converge_partial) it may +/// not be, and `converged` is what says so. #[derive(Clone, Debug)] -#[must_use = "a ConvergenceReport carries `converged`, and a fit that stopped \ - at `max_iter` is wrong by a little rather than loudly broken — \ - check it, or bind it to `_` to say you have decided not to"] +#[must_use = "from `converge_partial` this may describe a fit that stopped at \ + `max_iter`, which is wrong by a little rather than loudly \ + broken — check `converged`, or bind it to `_` to say you have \ + decided not to"] pub struct ConvergenceReport { pub iterations: usize, pub final_step: (f64, f64), diff --git a/src/error.rs b/src/error.rs index 612a041..de9d905 100644 --- a/src/error.rs +++ b/src/error.rs @@ -64,6 +64,24 @@ pub enum InferenceError { /// result has no representable likelihood. Configure a positive `p_draw` /// (via `HistoryBuilder::p_draw` or `GameOptions::p_draw`) to admit ties. TieWithoutDrawProbability { teams: (usize, usize) }, + /// The convergence sweep hit `max_iter` with the step still above + /// `epsilon`. + /// + /// A fit that stops short is wrong by a little, which is the worst + /// available failure: every rating is finite, the ordering looks sensible, + /// and nothing in the numbers says they were still moving. Reported rather + /// than returned as a flag on an `Ok`, because a flag has to be checked + /// and `let _ = h.converge()` is the natural way not to. + /// + /// Either the history needs more iterations — raise `max_iter` — or it is + /// oscillating rather than converging, in which case `alpha < 1.0` damps + /// the within-game EP loop. [`History::converge_partial`](crate::History::converge_partial) + /// returns the short fit instead when that is genuinely what is wanted. + NotConverged { + iterations: usize, + final_step: (f64, f64), + epsilon: f64, + }, /// Inference produced a non-finite value (NaN or infinity). /// /// Indicates numerical breakdown; the resulting skills are meaningless @@ -100,6 +118,17 @@ pub enum InferenceError { member: usize, key: String, }, + /// `History::register` was called for a competitor that already exists. + /// + /// Registration states a competitor's configuration before anything has + /// been observed about them, so a competitor that already exists has + /// already been configured — by an earlier `register`, or by an event that + /// created them. Silently overwriting would reintroduce exactly the + /// order-dependence registration exists to remove. + /// + /// To change an existing competitor's configuration, supply it on an event + /// through `Member`; that refits the whole history. + AlreadyRegistered { key: String }, /// A prediction was given a team with no members. EmptyTeam { team: usize }, /// A joint posterior was requested where one cannot be formed exactly. @@ -144,6 +173,18 @@ impl fmt::Display for InferenceError { teams.0, teams.1 ) } + Self::NotConverged { + iterations, + final_step, + epsilon, + } => { + write!( + f, + "did not converge in {iterations} iterations: final step {final_step:?} \ + is still above epsilon {epsilon}; raise max_iter, or damp with \ + alpha < 1.0 if it is oscillating" + ) + } Self::NonFiniteResult { context, step } => { write!( f, @@ -167,6 +208,14 @@ impl fmt::Display for InferenceError { with `lookup` or `current_skill` if that is not guaranteed)" ) } + Self::AlreadyRegistered { key } => { + write!( + f, + "competitor {key} is already registered; registration states \ + configuration before anything is observed, so re-registering \ + would silently overwrite it" + ) + } Self::EmptyTeam { team } => { write!(f, "team {team} has no members") } diff --git a/src/history.rs b/src/history.rs index 8046fad..a2a4c2c 100644 --- a/src/history.rs +++ b/src/history.rs @@ -6,6 +6,7 @@ use crate::{ convergence::{ConvergenceOptions, ConvergenceReport}, drift::{ConstantDrift, Drift}, error::InferenceError, + event::Member, gaussian::Gaussian, key_table::KeyTable, observer::{NullObserver, Observer}, @@ -39,17 +40,55 @@ pub struct HistoryBuilder< } impl, O: Observer, K: Eq + Hash + Clone> HistoryBuilder { + /// 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 } @@ -169,6 +208,7 @@ impl, O: Observer, K: Eq + Hash + Clone> HistoryBuilder< convergence: self.convergence, observer: self.observer, unknown_keys: self.unknown_keys, + declared: HashMap::new(), } } } @@ -276,6 +316,12 @@ pub struct History< convergence: ConvergenceOptions, observer: O, unknown_keys: crate::UnknownKeys, + /// Competitor configuration explicitly declared so far, by whichever route. + /// + /// Kept separate from the applied `Rating` because a `Rating` cannot say + /// whether a value was *chosen* or inherited from the history defaults, + /// and that is exactly the distinction a conflict check needs. + declared: HashMap, } impl Default for History { @@ -455,6 +501,111 @@ impl, O: Observer, K: Eq + Hash + Clone> History(()) + /// ``` + /// + /// The competitor exists from this point on, with no appearances, so + /// [`History::rating`] can read back what was actually stored — the + /// diagnostic that was previously missing entirely. + /// + /// `weight` is per-event and has no meaning here, so a `Member` carrying a + /// non-default one is rejected rather than silently ignored. + /// + /// # Errors + /// + /// `AlreadyRegistered` if the competitor already exists, whether from an + /// earlier `register` or from an event. `InvalidParameter` for a `weight` + /// other than 1.0, or a `drift_scale` that is negative or non-finite. + pub fn register(&mut self, member: Member) -> Result<(), InferenceError> + where + K: std::fmt::Debug, + { + if member.weight != 1.0 { + return Err(InferenceError::InvalidParameter { + name: "weight", + value: member.weight, + }); + } + if let Some(scale) = member.drift_scale { + if !scale.is_finite() || scale < 0.0 { + return Err(InferenceError::InvalidParameter { + name: "drift_scale", + value: scale, + }); + } + } + + let key = format!("{:?}", member.key); + let idx = self.keys.get_or_create(&member.key); + if self.agents.contains(idx) { + return Err(InferenceError::AlreadyRegistered { key }); + } + + let mut rating = Rating::new( + Gaussian::from_ms(self.mu, self.sigma), + self.beta, + self.drift, + ); + if let Some(prior) = member.prior { + rating.prior = prior; + } + if let Some(scale) = member.drift_scale { + rating.drift_scale = scale; + } + + self.declared.insert( + idx, + CompetitorConfig { + prior: member.prior, + drift_scale: member.drift_scale, + }, + ); + self.agents.insert( + idx, + Competitor { + rating, + message: None, + last_time: None, + }, + ); + + Ok(()) + } + + /// The configuration in force for a competitor, or `None` if the history + /// has never seen them. + /// + /// Reads back what was actually stored, which is what makes a + /// configuration mistake detectable from outside the crate. Every other + /// accessor returns what inference *inferred*; this returns what it was + /// told. + #[must_use] + pub fn rating(&self, key: &Q) -> Option> + where + K: std::borrow::Borrow, + Q: std::hash::Hash + Eq + ?Sized, + { + let idx = self.keys.get(key)?; + self.agents.contains(idx).then(|| self.agents[idx].rating) + } + pub fn current_skill(&self, key: &Q) -> Option where K: std::borrow::Borrow, @@ -958,6 +1109,14 @@ impl, O: Observer, K: Eq + Hash + Clone> History, O: Observer, K: Eq + Hash + Clone> History Result { + let report = self.converge_partial()?; + + if report.converged { + Ok(report) + } else { + Err(InferenceError::NotConverged { + iterations: report.iterations, + final_step: report.final_step, + epsilon: self.convergence.epsilon, + }) + } + } + + /// As [`History::converge`], but a fit that stops at `max_iter` is + /// returned rather than reported as an error. + /// + /// The report's `converged` flag says which happened. Use this when a + /// deliberately capped sweep is the point — a cheap approximate fit, or a + /// test that pins what a fixed number of iterations produces. Prefer + /// `converge` everywhere else: an unconverged fit that nobody checks is + /// indistinguishable from a converged one. + /// + /// # Errors + /// + /// `NonFiniteResult` if a sweep produces a NaN or infinite step. + pub fn converge_partial(&mut self) -> Result { use std::time::Instant; use smallvec::SmallVec; @@ -1564,6 +1768,47 @@ impl, O: Observer, K: Eq + Hash + Clone> History, O: Observer, K: Eq + Hash + Clone> History, O: Observer, K: Eq + Hash + Clone> History, O: Observer, K: Eq + Hash + Clone> { history: &'h History, cholesky: crate::joint::Cholesky, @@ -1993,9 +2268,14 @@ impl, O: Observer, K: Eq + Hash + Clone> Joint<'_, T, D, /// Number of variables in the joint: the history's appearances, after /// collapsing consecutive pairs a competitor does not drift between. /// - /// This is what the cost scales in, and it is not the competitor count — a - /// competitor contributes one variable per slice it appears in. Worth - /// checking before asking for a joint over a long history. + /// This is what the cost scales in — `O(n^3)` to factorise, `O(n^2)` per + /// query — and it is neither the competitor count nor slices times + /// competitors. A drift-free competitor contributes one variable however + /// many slices they appear in; see the type docs for how large that + /// difference gets. + /// + /// Worth reading before committing to a batch of queries: it is the one + /// number that says whether a joint over this history is affordable. #[must_use] pub fn variables(&self) -> usize { self.width @@ -2799,13 +3079,15 @@ mod tests { epsilon = 1e-6 ); - // run exactly 11 iterations (old test used convergence(11, ...)) + // Run exactly 11 iterations. `converge_partial` rather than + // `converge`: stopping at the cap is the point here, and `converge` + // now reports that as `NotConverged`. h.convergence = ConvergenceOptions { max_iter: 11, epsilon: EPSILON, alpha: 1.0, }; - let _ = h.converge().unwrap(); + let _ = h.converge_partial().unwrap(); let loocv_approx_2 = h.log_evidence_internal(false, &[]).exp().sqrt(); @@ -3172,7 +3454,9 @@ mod tests { }) .build(); events_for(&mut h_capped); - let _ = h_capped.converge().unwrap(); + // A one-iteration cap is deliberate here, so the short fit is the + // result rather than an error. + let _ = h_capped.converge_partial().unwrap(); let mut h_full: History = History::builder().build(); events_for(&mut h_full); diff --git a/src/lib.rs b/src/lib.rs index a918605..a337b45 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -158,22 +158,43 @@ pub const P_DRAW: f64 = 0.0; pub const EPSILON: f64 = 1e-6; /// Default cap on convergence sweeps. /// -/// **This is a floor, not a recommendation.** It is adequate for small -/// histories and is quickly outgrown: a history of 400 events over 100 -/// competitors already stops here with a final step of ~7e-3 against the 1e-6 -/// default tolerance — four orders of magnitude short — and a dense joint model -/// of ~2,000 nodes over ~3,300 events has been measured needing 76 to 161. +/// **A runaway guard, not a budget.** The sweep exits as soon as the step falls +/// below `epsilon`, so the cap is never reached by a history that converges and +/// raising it costs nothing. Measured on a history that needs four sweeps: /// -/// Overrunning it is not an error, and deliberately so: `converge` returns a -/// [`ConvergenceReport`] whose `converged` flag says what happened. But a fit -/// that stopped short is *wrong by a little*, which is the worst available -/// failure — every rating is finite and ordered sensibly, and nothing in the -/// numbers themselves says they were still moving. Read the report; the type is -/// `#[must_use]` for that reason. +/// ```text +/// max_iter 30: 4 iterations, 129.9 us +/// max_iter 100_000: 4 iterations, 131.9 us +/// ``` /// -/// Raise it via [`ConvergenceOptions`]. Convergence cost is roughly linear in -/// the cap, and for anything but a toy the extra sweeps are milliseconds. -pub const ITERATIONS: usize = 30; +/// This was `30` until it was measured, and 30 truncated ordinary healthy +/// histories: 160 events over 100 competitors already needs 42. Because a short +/// fit is finite and sensibly ordered, that was invisible. +/// +/// # Why it is not scaled to the history +/// +/// The obvious improvement — pick the cap from the node or event count — does +/// not work, because iteration count is driven by how *loopy* the graph is +/// rather than how big it is. At a fixed 320 events over 40 slices, varying +/// only the number of competitors sharing them: +/// +/// ```text +/// competitors appearances each iterations +/// 3 213 2_789 +/// 10 64 1_068 +/// 50 12.8 206 +/// 100 6.4 90 +/// 400 1.6 2 +/// ``` +/// +/// Three orders of magnitude apart on identical event and slice counts. Any +/// formula in those two numbers would be badly wrong on some real shape, so the +/// cap is a single value set high enough that reaching it means the fit is +/// oscillating rather than merely large. +/// +/// Reaching it is [`InferenceError::NotConverged`]. See +/// [`History::converge`](crate::History::converge). +pub const ITERATIONS: usize = 10_000; /// Largest team count `History::predict_outcome` will enumerate. /// diff --git a/tests/convergence_strictness.rs b/tests/convergence_strictness.rs new file mode 100644 index 0000000..7d6027c --- /dev/null +++ b/tests/convergence_strictness.rs @@ -0,0 +1,152 @@ +//! Stopping short of convergence is an error, not a flag on a success. +//! +//! A fit that hits `max_iter` is wrong by a little: every rating is finite, +//! the ordering looks sensible, and nothing in the numbers says they were +//! still moving. When that was `Ok` with `converged: false`, detecting it was +//! opt-in and `let _ = h.converge()` was the natural way to opt out — which is +//! how a real defect once hid in this crate's own suite. + +use smallvec::smallvec; +use trueskill_tt::{ + ConstantDrift, ConvergenceOptions, Event, History, InferenceError, Member, Outcome, Team, +}; + +type H = History; + +fn duel(a: &'static str, b: &'static str, t: i64) -> Event { + Event { + time: t, + teams: smallvec![ + Team::with_members([Member::new(a)]), + Team::with_members([Member::new(b)]), + ], + outcome: Outcome::scores([3.0, 1.0]), + } +} + +fn capped(max_iter: usize) -> H { + History::builder() + .mu(0.0) + .sigma(6.0) + .beta(1.0) + .score_sigma(2.0) + .drift(ConstantDrift(0.5)) + .convergence(ConvergenceOptions { + max_iter, + epsilon: 1e-13, + alpha: 1.0, + }) + .build() +} + +fn fill(h: &mut H) { + h.add_events((1..=6).map(|t| duel("a", "b", t)).collect::>()) + .unwrap(); +} + +#[test] +fn hitting_the_cap_is_an_error() { + let mut h = capped(1); + fill(&mut h); + let err = h.converge().unwrap_err(); + match err { + InferenceError::NotConverged { + iterations, + final_step, + epsilon, + } => { + assert_eq!(iterations, 1); + assert!( + final_step.0 > epsilon || final_step.1 > epsilon, + "{final_step:?}" + ); + } + other => panic!("expected NotConverged, got {other:?}"), + } +} + +/// The message has to name what to do about it, since the fit looks fine. +#[test] +fn the_error_says_how_to_fix_it() { + let mut h = capped(1); + fill(&mut h); + let text = h.converge().unwrap_err().to_string(); + assert!(text.contains("did not converge in 1 iterations"), "{text}"); + assert!(text.contains("max_iter"), "{text}"); + assert!(text.contains("alpha"), "{text}"); +} + +/// The escape hatch: a deliberately capped fit is still reachable. +#[test] +fn converge_partial_returns_the_short_fit() { + let mut h = capped(1); + fill(&mut h); + let report = h.converge_partial().unwrap(); + assert_eq!(report.iterations, 1); + assert!(!report.converged); + assert!(h.current_skill(&"a").is_some()); +} + +/// Both agree when the fit does converge, so the strict path costs nothing. +#[test] +fn the_two_agree_on_a_converged_fit() { + let mut strict = capped(20_000); + fill(&mut strict); + let a = strict.converge().unwrap(); + + let mut partial = capped(20_000); + fill(&mut partial); + let b = partial.converge_partial().unwrap(); + + assert!(a.converged && b.converged); + assert_eq!(a.iterations, b.iterations); + assert_eq!(a.final_step, b.final_step); +} + +/// The default cap must be high enough that an ordinary history clears it. +/// At the old value of 30 this history stopped short and said nothing. +#[test] +fn the_default_cap_clears_an_ordinary_history() { + let mut h: History = History::builder_with_key() + .mu(0.0) + .sigma(6.0) + .beta(1.0) + .score_sigma(2.0) + .drift(ConstantDrift(0.05)) + .build(); + + let mut events = Vec::new(); + for t in 0..20i64 { + for j in 0..8usize { + let k = (t as usize) * 8 + j; + events.push(Event { + time: t, + teams: smallvec![ + Team::with_members([Member::new(format!("p{}", k % 100))]), + Team::with_members([Member::new(format!("p{}", (k + 37) % 100))]), + ], + outcome: Outcome::scores([3.0, 1.0]), + }); + } + } + h.add_events(events).unwrap(); + + let report = h + .converge() + .expect("an ordinary history must converge by default"); + assert!( + report.iterations > 30, + "needed {} sweeps", + report.iterations + ); + assert!(report.iterations < trueskill_tt::ITERATIONS); +} + +/// An empty history converges trivially rather than erroring. +#[test] +fn an_empty_history_converges() { + let mut h = capped(1); + let report = h.converge().unwrap(); + assert!(report.converged); + assert_eq!(report.iterations, 0); +} diff --git a/tests/joint_handle.rs b/tests/joint_handle.rs index f93ff77..e5d90df 100644 --- a/tests/joint_handle.rs +++ b/tests/joint_handle.rs @@ -141,6 +141,53 @@ fn variables_counts_appearances_not_competitors() { assert_eq!(joint.variables(), 12); } +/// How much the collapse is worth, which is the part a caller has to plan +/// around: a drift-free competitor contributes **one** variable however long +/// the history, so the same events at `gamma = 0` and `gamma > 0` differ by +/// roughly the slice count in problem size — and by its cube in solve time. +/// +/// Reported by a consumer as an 8x difference in solve time on a ~2,000-node, +/// 76-slice model (787 ms career against 6,214 ms drifting). This pins the +/// mechanism behind that so a change to the collapse rule cannot quietly +/// remove it. +#[test] +fn drift_free_competitors_shrink_the_joint_by_the_slice_count() { + fn variables(gamma: f64) -> usize { + let mut h = History::builder() + .mu(0.0) + .sigma(6.0) + .beta(1.0) + .score_sigma(2.0) + .drift(ConstantDrift(gamma)) + .convergence(ConvergenceOptions { + max_iter: 20_000, + epsilon: 1e-13, + alpha: 1.0, + }) + .build(); + h.add_events( + (1..=10) + .map(|t| duel("a", "b", t, 5.0, 2.0)) + .collect::>(), + ) + .unwrap(); + let _ = h.converge().unwrap(); + h.joint().unwrap().variables() + } + + let drifting = variables(0.5); + let career = variables(0.0); + + // Two competitors over ten slices: twenty appearances, or two variables. + assert_eq!(drifting, 20); + assert_eq!(career, 2); + assert_eq!( + drifting / career, + 10, + "collapse should track the slice count" + ); +} + /// With `drift = 0` consecutive appearances are the same latent variable, so /// the joint is smaller than the appearance count. #[test] diff --git a/tests/registration.rs b/tests/registration.rs new file mode 100644 index 0000000..df7dee4 --- /dev/null +++ b/tests/registration.rs @@ -0,0 +1,338 @@ +//! Configuring a competitor before anything is observed about them. +//! +//! The configuration a competitor needs is usually a property of the domain — +//! "every layout is static" — not of whichever event happens to mention them +//! first. Stating it per-event meant every ingestion path had to remember it, +//! and two of the four paths could not state it at all. + +use smallvec::smallvec; +use trueskill_tt::{ + ConstantDrift, ConvergenceOptions, Event, Gaussian, History, InferenceError, Member, Outcome, + Team, +}; + +type H = History; + +const PINNED: Gaussian = Gaussian::from_ms(2.0, 0.5); + +fn history() -> H { + History::builder() + .mu(0.0) + .sigma(6.0) + .beta(1.0) + .score_sigma(2.0) + .drift(ConstantDrift(0.5)) + .convergence(ConvergenceOptions { + max_iter: 20_000, + epsilon: 1e-13, + alpha: 1.0, + }) + .build() +} + +fn duel( + a: &'static str, + b: &'static str, + t: i64, + m: Option>, +) -> Event { + Event { + time: t, + teams: smallvec![ + Team::with_members([Member::new(a)]), + Team::with_members([m.unwrap_or_else(|| Member::new(b))]), + ], + outcome: Outcome::scores([5.0, 2.0]), + } +} + +fn skills(h: &H) -> Vec<(&'static str, Gaussian)> { + ["player", "layout"] + .into_iter() + .map(|k| (k, h.current_skill(&k).unwrap())) + .collect() +} + +/// The headline contract. +#[test] +fn registering_matches_configuring_on_the_first_event() { + let configured = { + let mut h = history(); + h.add_events(vec![ + duel( + "player", + "layout", + 1, + Some( + Member::new("layout") + .with_drift_scale(0.0) + .with_prior(PINNED), + ), + ), + duel("player", "layout", 2, None), + ]) + .unwrap(); + let _ = h.converge().unwrap(); + h + }; + + let registered = { + let mut h = history(); + h.register( + Member::new("layout") + .with_drift_scale(0.0) + .with_prior(PINNED), + ) + .unwrap(); + h.add_events(vec![ + duel("player", "layout", 1, None), + duel("player", "layout", 2, None), + ]) + .unwrap(); + let _ = h.converge().unwrap(); + h + }; + + for ((k, a), (_, b)) in skills(&configured).into_iter().zip(skills(®istered)) { + assert_eq!(a.pi(), b.pi(), "{k} pi"); + assert_eq!(a.tau(), b.tau(), "{k} tau"); + } +} + +/// The case `EventBuilder` and the typed path cannot reach: a competitor whose +/// first appearance arrives through the two-argument convenience route. +#[test] +fn registration_reaches_a_competitor_first_seen_through_record_winner() { + let mut h = history(); + h.register( + Member::new("layout") + .with_drift_scale(0.0) + .with_prior(PINNED), + ) + .unwrap(); + h.record_winner(&"player", &"layout", 1).unwrap(); + h.record_winner(&"player", &"layout", 2).unwrap(); + let _ = h.converge().unwrap(); + + let rating = h.rating(&"layout").unwrap(); + assert_eq!(rating.drift_scale(), 0.0); + assert_eq!(rating.prior().mu(), PINNED.mu()); + + // Pinned means pinned: no drift across the two slices. + let curve = h.learning_curve(&"layout"); + assert!(curve.len() >= 2); + let widest = curve + .iter() + .map(|(_, g)| g.sigma()) + .fold(f64::MIN, f64::max); + let narrowest = curve + .iter() + .map(|(_, g)| g.sigma()) + .fold(f64::MAX, f64::min); + assert!( + (widest - narrowest) / widest < 1e-9, + "{narrowest} .. {widest}" + ); +} + +#[test] +fn registering_a_known_competitor_is_an_error() { + let mut h = history(); + h.record_winner(&"player", &"layout", 1).unwrap(); + let err = h.register(Member::new("layout")).unwrap_err(); + assert!( + matches!(err, InferenceError::AlreadyRegistered { .. }), + "{err:?}" + ); +} + +#[test] +fn registering_twice_is_an_error() { + let mut h = history(); + h.register(Member::new("layout").with_drift_scale(0.0)) + .unwrap(); + let err = h + .register(Member::new("layout").with_drift_scale(1.0)) + .unwrap_err(); + assert!( + matches!(err, InferenceError::AlreadyRegistered { .. }), + "{err:?}" + ); + // The first registration stands. + assert_eq!(h.rating(&"layout").unwrap().drift_scale(), 0.0); +} + +/// `weight` is per-event and meaningless here, so it is rejected rather than +/// dropped — dropping it silently is the defect class this whole area keeps +/// producing. +#[test] +fn a_weight_on_a_registration_is_rejected() { + let mut h = history(); + let err = h + .register(Member::new("layout").with_weight(0.5)) + .unwrap_err(); + assert!( + matches!(err, InferenceError::InvalidParameter { name: "weight", .. }), + "{err:?}" + ); +} + +#[test] +fn an_invalid_drift_scale_on_a_registration_is_rejected() { + for bad in [-1.0, f64::NAN, f64::INFINITY] { + let mut h = history(); + let err = h + .register(Member::new("layout").with_drift_scale(bad)) + .unwrap_err(); + assert!( + matches!( + err, + InferenceError::InvalidParameter { + name: "drift_scale", + .. + } + ), + "{bad}: {err:?}" + ); + } +} + +/// Registration makes the fit independent of the order events arrive in, +/// which is what the per-event shape could not guarantee. +#[test] +fn registration_makes_the_fit_order_independent() { + let build = |reversed: bool| { + let mut h = history(); + h.register( + Member::new("layout") + .with_drift_scale(0.0) + .with_prior(PINNED), + ) + .unwrap(); + let mut events = vec![ + duel("player", "layout", 1, None), + duel("player", "layout", 2, None), + duel("player", "layout", 3, None), + ]; + if reversed { + events.reverse(); + } + h.add_events(events).unwrap(); + let _ = h.converge().unwrap(); + h + }; + + let forward = build(false); + let backward = build(true); + for ((k, a), (_, b)) in skills(&forward).into_iter().zip(skills(&backward)) { + assert_eq!(a.pi(), b.pi(), "{k} pi"); + assert_eq!(a.tau(), b.tau(), "{k} tau"); + } +} + +/// `rating` is the read-back that made a configuration mistake detectable from +/// outside the crate at all. Every other accessor reports what inference +/// inferred; this reports what it was told. +#[test] +fn rating_reads_back_what_was_stored() { + let mut h = history(); + assert!(h.rating(&"nobody").is_none()); + + h.register( + Member::new("layout") + .with_drift_scale(0.25) + .with_prior(PINNED), + ) + .unwrap(); + let r = h.rating(&"layout").unwrap(); + assert_eq!(r.drift_scale(), 0.25); + assert_eq!(r.prior().pi(), PINNED.pi()); + assert_eq!(r.prior().tau(), PINNED.tau()); + + // A competitor created by an event reports the history defaults. + h.record_winner(&"player", &"layout", 1).unwrap(); + assert_eq!(h.rating(&"player").unwrap().drift_scale(), 1.0); +} + +/// The decision this issue turned on: two different values for one competitor +/// are an error whether they arrive in one batch or two. +/// +/// Last-write-wins across batches cut against the invariant +/// `tests/ingestion_equivalence.rs` protects — the same contradictory events +/// errored when batched and succeeded, order-dependently, one at a time. +mod conflicting_configuration { + use super::*; + + fn seed(scale: f64) -> Event { + duel( + "player", + "layout", + 1, + Some(Member::new("layout").with_drift_scale(scale)), + ) + } + + #[test] + fn within_one_batch_is_an_error() { + let mut h = history(); + let err = h.add_events(vec![seed(0.0), seed(1.0)]).unwrap_err(); + assert!( + matches!( + err, + InferenceError::ConflictingCompetitorConfig { + field: "drift_scale", + .. + } + ), + "{err:?}" + ); + } + + #[test] + fn across_two_batches_is_also_an_error() { + let mut h = history(); + h.add_events(vec![seed(0.0)]).unwrap(); + let err = h.add_events(vec![seed(1.0)]).unwrap_err(); + assert!( + matches!( + err, + InferenceError::ConflictingCompetitorConfig { + field: "drift_scale", + .. + } + ), + "{err:?}" + ); + // Rejected before anything mutates: the first declaration stands. + assert_eq!(h.rating(&"layout").unwrap().drift_scale(), 0.0); + } + + /// Repeating the *same* value stays inert, which is the expected shape + /// when the configuration is a property of the domain. + #[test] + fn repeating_the_same_value_is_inert() { + let mut h = history(); + h.add_events(vec![seed(0.0)]).unwrap(); + h.add_events(vec![seed(0.0)]).unwrap(); + assert_eq!(h.rating(&"layout").unwrap().drift_scale(), 0.0); + } + + /// A registration and a later event that agree are fine; one that + /// disagrees is the same error. + #[test] + fn a_registration_conflicts_with_a_later_event() { + let mut h = history(); + h.register(Member::new("layout").with_drift_scale(0.0)) + .unwrap(); + h.add_events(vec![seed(0.0)]).unwrap(); + + let mut h2 = history(); + h2.register(Member::new("layout").with_drift_scale(0.0)) + .unwrap(); + let err = h2.add_events(vec![seed(1.0)]).unwrap_err(); + assert!( + matches!(err, InferenceError::ConflictingCompetitorConfig { .. }), + "{err:?}" + ); + } +} diff --git a/tests/validation.rs b/tests/validation.rs index 2bb9cea..9ec4686 100644 --- a/tests/validation.rs +++ b/tests/validation.rs @@ -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:?}" + ); + } +}