From eff63dfa2a944e7a4ad86f4298cd9ec8dd727111 Mon Sep 17 00:00:00 2001 From: Anders Olsson Date: Tue, 8 Sep 2026 16:04:22 +0200 Subject: [PATCH 1/4] feat!: make a short fit an error and raise the default iteration cap MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `ITERATIONS` was 30, and overrunning it returned `Ok` with `converged: false`. Both halves were wrong. The cap is a runaway guard, not a budget: the sweep exits as soon as the step falls below `epsilon`, so a high cap costs nothing on a history that converges. Measured on one needing four sweeps, `max_iter` 30 and 100_000 both finish in 4 iterations and ~130us. So 30 could never make anything faster — it could only stop a healthy history early, and it did: 160 events over 100 competitors already needs 42. Not scaled to the history, because iteration count tracks how loopy the graph is rather than how big it is. At a fixed 320 events over 40 slices, varying only the competitors sharing them: 3 competitors needs 2_789 sweeps, 10 needs 1_068, 100 needs 90, 400 needs 2. Three orders of magnitude on identical event and slice counts, so any formula in those two numbers would be badly wrong on some real shape. A single value set high enough that reaching it means oscillation is the honest version. With the cap raised, stopping at it means something is genuinely wrong, so `converge` now returns `InferenceError::NotConverged` rather than a flag on a success. A short fit is wrong by a little — every rating finite, the ordering sensible, nothing saying the numbers were still moving — and a flag has to be checked while `let _ = h.converge()` is the natural way not to. That is not hypothetical: it is how a real defect hid in this crate's own test suite. `converge_partial` returns the short fit for callers who want one. Only a single existing test needed it, which is the evidence that a capped fit is a deliberate choice rather than the common case. Also corrects the `ITERATIONS` docs, which claimed convergence cost is "roughly linear in the cap". It is linear in the iterations actually run. BREAKING CHANGE: `History::converge` returns `Err(NotConverged)` where it previously returned `Ok` with `converged: false`. Callers that want the old behaviour should use `History::converge_partial`. The default `max_iter` changes from 30 to 10_000, so a history that was silently truncated will now converge properly and its numbers will move. Closes #50 Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_011hcFjNDmHXZF8URGLku5zZ --- src/convergence.rs | 13 ++- src/error.rs | 30 +++++++ src/history.rs | 61 +++++++++++-- src/lib.rs | 49 +++++++--- tests/convergence_strictness.rs | 152 ++++++++++++++++++++++++++++++++ 5 files changed, 282 insertions(+), 23 deletions(-) create mode 100644 tests/convergence_strictness.rs 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..d0dfb01 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 @@ -144,6 +162,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, diff --git a/src/history.rs b/src/history.rs index 8046fad..e904a43 100644 --- a/src/history.rs +++ b/src/history.rs @@ -1390,17 +1390,62 @@ impl, 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; @@ -2799,13 +2844,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 +3219,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); +} From 4f6360128de1998e6031ba5eaf7287c97c43e07d Mon Sep 17 00:00:00 2001 From: Anders Olsson Date: Tue, 8 Sep 2026 16:06:37 +0200 Subject: [PATCH 2/4] feat!: validate mu, sigma and beta on HistoryBuilder MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) Claude-Session: https://claude.ai/code/session_011hcFjNDmHXZF8URGLku5zZ --- src/history.rs | 38 ++++++++++++++++++++++++ tests/validation.rs | 71 +++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 109 insertions(+) diff --git a/src/history.rs b/src/history.rs index e904a43..40befe9 100644 --- a/src/history.rs +++ b/src/history.rs @@ -39,17 +39,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 } 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:?}" + ); + } +} From e493f47e993a693c7d0b83bab62946167e0f7a17 Mon Sep 17 00:00:00 2001 From: Anders Olsson Date: Tue, 8 Sep 2026 16:11:09 +0200 Subject: [PATCH 3/4] feat!: add History::register and History::rating, and reject config conflicts across batches MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three things #38 asked for, on a premise that had half dissolved. The issue argued from "captured at first appearance", "missing it is silent" and "missing it is permanent"; 8c087ad made configuration apply whenever supplied and refit the whole history, so two of those are already gone. What survived is the literal title — no way to say it before the first event — plus the absence of any way to check. `register(Member)` states configuration before anything is observed. It takes the same `Member` ingestion takes, so there is one vocabulary rather than two, and it creates the competitor immediately, which is what makes it observable. It reaches a competitor first seen through `record_winner`, the route #37 deliberately did not extend. `rating(&key)` reads back what was stored. Every other accessor reports what inference inferred; this reports what it was told, which is what makes a configuration mistake detectable from outside the crate at all. Conflicting configuration is now an error across batches, not only within one. The `priors` map is rebuilt per `add_events` call, so a second batch silently overwrote what a first declared, last-write-wins. That cut directly against the invariant tests/ingestion_equivalence.rs exists to protect: the same contradictory events errored when batched and succeeded, order-dependently, when fed one at a time. Detection lives on a new `declared` map on `History`, because a `Rating` cannot say whether a value was chosen or inherited from the defaults — which is exactly the distinction the check needs. Checked before anything mutates, so a rejected batch leaves the history untouched. `register` rejects a non-default `weight` rather than ignoring it. Weight is per-event and has no meaning on a registration, and silently dropping a field the caller set is the defect this whole area keeps producing. The declarative `default_rating_for` closure is not here. It is the better answer for ustat's actual case — thousands of keys matching a rule, rather than enumerated — but it adds a `Fn` parameter to `History`, which the issue itself flags as in tension with the crate's posture. That wants its own decision rather than riding along. BREAKING CHANGE: two different values for one competitor's `prior` or `drift_scale` supplied across separate `add_events` calls now return `ConflictingCompetitorConfig` instead of silently taking the later one. Refs #38 Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_011hcFjNDmHXZF8URGLku5zZ --- src/error.rs | 19 +++ src/history.rs | 158 +++++++++++++++++++- tests/registration.rs | 338 ++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 514 insertions(+), 1 deletion(-) create mode 100644 tests/registration.rs diff --git a/src/error.rs b/src/error.rs index d0dfb01..de9d905 100644 --- a/src/error.rs +++ b/src/error.rs @@ -118,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. @@ -197,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 40befe9..c769d97 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}, @@ -207,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(), } } } @@ -314,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 { @@ -493,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, @@ -1647,6 +1760,47 @@ impl, O: Observer, K: Eq + Hash + Clone> History, O: Observer, K: Eq + Hash + Clone> 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:?}" + ); + } +} From f692906ce463ff85078be5f1cb5204a63ec768a8 Mon Sep 17 00:00:00 2001 From: Anders Olsson Date: Tue, 8 Sep 2026 19:45:51 +0200 Subject: [PATCH 4/4] docs: state what the joint's cost actually scales in MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A consumer measured an 8x difference in solve time between two fits over the same events, the same slices and the same ~2,000 nodes: career fit (gamma = 0) 787 ms drifting fit (gamma = 0.15) 6214 ms Entirely the collapse rule. A competitor with zero drift contributes one variable however long the history, so a drift-free fit's joint is smaller than a drifting one's by roughly the slice count — and to factorise, by its cube. Choosing a drift configuration is therefore also choosing a query cost, and nothing said so. Documented on `Joint`, on `Joint::variables` and on `posterior_of`, with the measurement. `variables()` is named as the number that decides affordability, since it can be read before committing to a batch. Also states the thing the consumer proposed as a future optimisation, because it is already true: an absence is not an appearance, so a competitor seen in the first and last of a hundred slices contributes two variables rather than a hundred. The matrix is already as small as the model allows on that axis. tests/joint_handle.rs pins the mechanism — ten slices, two competitors, twenty variables drifting against two at `gamma = 0` — so a change to the collapse rule cannot quietly remove the property the docs now promise. Refs #51 Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_011hcFjNDmHXZF8URGLku5zZ --- src/history.rs | 47 ++++++++++++++++++++++++++++++++++++++++--- tests/joint_handle.rs | 47 +++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 91 insertions(+), 3 deletions(-) diff --git a/src/history.rs b/src/history.rs index c769d97..a2a4c2c 100644 --- a/src/history.rs +++ b/src/history.rs @@ -1109,6 +1109,14 @@ impl, 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, @@ -2232,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 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]