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); +}