diff --git a/src/color_group.rs b/src/color_group.rs index b33ea41..8898fdd 100644 --- a/src/color_group.rs +++ b/src/color_group.rs @@ -26,23 +26,22 @@ pub(crate) struct ColorGroups { } impl ColorGroups { - #[allow(dead_code)] pub(crate) fn new() -> Self { Self::default() } - #[allow(dead_code)] - pub(crate) fn n_colors(&self) -> usize { - self.groups.len() - } - - #[allow(dead_code)] pub(crate) fn is_empty(&self) -> bool { self.groups.is_empty() } - /// Total event count across all colors. - #[allow(dead_code)] + /// Number of distinct colors in the partition. Test-only. + #[cfg(test)] + pub(crate) fn n_colors(&self) -> usize { + self.groups.len() + } + + /// Total event count across all colors. Test-only. + #[cfg(test)] pub(crate) fn total_events(&self) -> usize { self.groups.iter().map(|g| g.len()).sum() } diff --git a/src/game.rs b/src/game.rs index 8dd5f16..7fc8f84 100644 --- a/src/game.rs +++ b/src/game.rs @@ -88,16 +88,12 @@ impl Default for GameOptions { /// Owned variant of `Game` returned by public constructors. /// /// Unlike `Game<'a, T, D>` (which borrows its result/weights slices from -/// History's internal state), `OwnedGame` owns its inputs so it can -/// be returned freely from public constructors. +/// History's internal state), `OwnedGame` owns the team ratings, so it +/// can be returned freely from public constructors. The inference inputs +/// themselves are not retained — nothing reads them back. #[derive(Debug)] -#[allow(dead_code)] pub struct OwnedGame> { teams: Vec>>, - result: Vec, - weights: Vec>, - p_draw: f64, - pub(crate) convergence: crate::ConvergenceOptions, pub(crate) likelihoods: Vec>, pub(crate) log_evidence: f64, } @@ -119,16 +115,10 @@ impl> OwnedGame { convergence, &mut arena, ); - let likelihoods = g.likelihoods; - let log_evidence = g.log_evidence; Self { teams, - result, - weights, - p_draw, - convergence, - likelihoods, - log_evidence, + likelihoods: g.likelihoods, + log_evidence: g.log_evidence, } } @@ -148,16 +138,10 @@ impl> OwnedGame { convergence, &mut arena, ); - let likelihoods = g.likelihoods; - let log_evidence = g.log_evidence; Self { teams, - result: scores, - weights, - p_draw: 0.0, - convergence, - likelihoods, - log_evidence, + likelihoods: g.likelihoods, + log_evidence: g.log_evidence, } } diff --git a/src/history.rs b/src/history.rs index 8690e44..ee8b904 100644 --- a/src/history.rs +++ b/src/history.rs @@ -69,7 +69,20 @@ impl, O: Observer, K: Eq + Hash + Clone> HistoryBuilder< } } + /// Probability that two evenly-matched sides draw. + /// + /// Must be in `[0.0, 1.0)`. A zero draw probability asserts that draws + /// cannot occur, so ingesting a tied outcome then fails with + /// `InferenceError::TieWithoutDrawProbability`. + /// + /// # Panics + /// + /// Panics if `p_draw` is outside `[0.0, 1.0)` or is NaN. pub fn p_draw(mut self, p_draw: f64) -> Self { + assert!( + (0.0..1.0).contains(&p_draw), + "p_draw must be in [0.0, 1.0) (got {p_draw})" + ); self.p_draw = p_draw; self } @@ -79,6 +92,11 @@ impl, O: Observer, K: Eq + Hash + Clone> HistoryBuilder< self } + /// Default observation noise for scored outcomes. + /// + /// # Panics + /// + /// Panics if `score_sigma` is not strictly positive. pub fn score_sigma(mut self, score_sigma: f64) -> Self { assert!( score_sigma > 0.0, @@ -88,7 +106,24 @@ impl, O: Observer, K: Eq + Hash + Clone> HistoryBuilder< self } + /// Convergence tolerance, iteration cap, and EP damping. + /// + /// # Panics + /// + /// Panics if `alpha` is outside `(0.0, 1.0]`, or if `epsilon` is negative + /// or NaN. An `alpha` of zero would leave every EP update unapplied, so + /// inference would silently return the priors. pub fn convergence(mut self, opts: ConvergenceOptions) -> Self { + assert!( + opts.alpha > 0.0 && opts.alpha <= 1.0, + "convergence alpha must be in (0.0, 1.0] (got {})", + opts.alpha + ); + assert!( + opts.epsilon >= 0.0, + "convergence epsilon must be non-negative (got {})", + opts.epsilon + ); self.convergence = opts; self } diff --git a/src/lib.rs b/src/lib.rs index ae109f5..c019867 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -39,7 +39,7 @@ pub use event::{Event, Member, Team}; pub use event_builder::EventBuilder; pub use game::{Game, GameOptions, OwnedGame}; pub use gaussian::Gaussian; -pub use history::History; +pub use history::{History, HistoryBuilder}; pub use key_table::KeyTable; use matrix::Matrix; pub use observer::{NullObserver, Observer}; @@ -65,12 +65,29 @@ pub const N_INF: Gaussian = Gaussian::from_ms(0.0, f64::INFINITY); #[derive(Copy, Clone, Default, PartialEq, PartialOrd, Eq, Ord, Hash, Debug)] pub struct Index(usize); +impl Index { + /// The underlying slot number. + /// + /// Indices are dense and assigned in interning order, so this is usable as + /// a key into a caller-side side table. + #[must_use] + pub fn get(self) -> usize { + self.0 + } +} + impl From for Index { fn from(ix: usize) -> Self { Self(ix) } } +impl From for usize { + fn from(idx: Index) -> Self { + idx.0 + } +} + fn erfc(x: f64) -> f64 { let z = x.abs(); let t = 1.0 / (1.0 + z / 2.0); diff --git a/src/rating.rs b/src/rating.rs index 3530e24..6b75d28 100644 --- a/src/rating.rs +++ b/src/rating.rs @@ -29,6 +29,24 @@ impl> Rating { } } + /// The configured prior skill estimate. + #[must_use] + pub fn prior(&self) -> Gaussian { + self.prior + } + + /// Performance noise: how much a single showing varies around the skill. + #[must_use] + pub fn beta(&self) -> f64 { + self.beta + } + + /// The drift model governing how skill may move between events. + #[must_use] + pub fn drift(&self) -> D { + self.drift + } + pub(crate) fn performance(&self) -> Gaussian { self.prior.forget(self.beta.powi(2)) } diff --git a/src/schedule.rs b/src/schedule.rs index a08d20c..a0606b3 100644 --- a/src/schedule.rs +++ b/src/schedule.rs @@ -32,8 +32,17 @@ pub struct EpsilonOrMax { impl Default for EpsilonOrMax { fn default() -> Self { - // Matches today's hard-coded tolerance and iteration cap. - Self { eps: 1e-6, max: 10 } + // Derived from `ConvergenceOptions` so there is one source of truth for + // the tolerance and iteration cap. These previously disagreed: this + // default capped at 10 iterations while `ConvergenceOptions` allowed 30, + // and which applied depended on whether inference went through + // `run_chain` or a `Schedule`. + let defaults = crate::ConvergenceOptions::default(); + + Self { + eps: defaults.epsilon, + max: defaults.max_iter, + } } } @@ -50,10 +59,16 @@ impl Schedule for EpsilonOrMax { } let mut iterations = 0; - let mut final_step = (f64::INFINITY, f64::INFINITY); - let mut converged = false; + // With no iterating factors the graph is already at its fixed point: + // the setup pass above is all there is to do. Reporting `converged: + // false` with an infinite step for that case gave callers a false + // negative. + let mut final_step = (0.0, 0.0); + let mut converged = true; if n_setup < factors.len() { + final_step = (f64::INFINITY, f64::INFINITY); + converged = false; for _ in 0..self.max { let mut step = (0.0_f64, 0.0_f64); @@ -113,7 +128,8 @@ mod tests { #[test] fn report_marks_converged_when_no_iterating_factors() { - // No iterating factors → 0 iterations, converged stays false (loop never ran). + // A graph of only setup factors has nothing to iterate, so it is at its + // fixed point after the setup pass: 0 iterations, and converged. let mut vars = VarStore::new(); let out = vars.alloc(N_INF); let mut factors = vec![BuiltinFactor::TeamSum(TeamSumFactor { @@ -122,5 +138,15 @@ mod tests { })]; let report = EpsilonOrMax::default().run(&mut factors, &mut vars); assert_eq!(report.iterations, 0); + assert!(report.converged); + assert_eq!(report.final_step, (0.0, 0.0)); + } + + #[test] + fn default_matches_convergence_options() { + let schedule = EpsilonOrMax::default(); + let options = crate::ConvergenceOptions::default(); + assert_eq!(schedule.max, options.max_iter); + assert_eq!(schedule.eps, options.epsilon); } } diff --git a/src/storage/skill_store.rs b/src/storage/skill_store.rs index 0c3632f..5732641 100644 --- a/src/storage/skill_store.rs +++ b/src/storage/skill_store.rs @@ -41,6 +41,18 @@ impl SkillStore { } } + /// Whether a slot is occupied. Test-only. + #[cfg(test)] + pub fn contains(&self, idx: Index) -> bool { + idx.0 < self.present.len() && self.present[idx.0] + } + + /// Number of occupied slots. Test-only. + #[cfg(test)] + pub fn len(&self) -> usize { + self.n_present + } + pub fn get_mut(&mut self, idx: Index) -> Option<&mut Skill> { if idx.0 < self.present.len() && self.present[idx.0] { Some(&mut self.skills[idx.0]) @@ -49,21 +61,6 @@ impl SkillStore { } } - #[allow(dead_code)] - pub fn contains(&self, idx: Index) -> bool { - idx.0 < self.present.len() && self.present[idx.0] - } - - #[allow(dead_code)] - pub fn len(&self) -> usize { - self.n_present - } - - #[allow(dead_code)] - pub fn is_empty(&self) -> bool { - self.n_present == 0 - } - pub fn iter(&self) -> impl Iterator { self.present.iter().enumerate().filter_map(|(i, &p)| { if p { diff --git a/src/time_slice.rs b/src/time_slice.rs index b762a81..8c95927 100644 --- a/src/time_slice.rs +++ b/src/time_slice.rs @@ -14,7 +14,6 @@ use crate::{ rating::Rating, storage::{CompetitorStore, SkillStore}, time::Time, - tuple_gt, tuple_max, }; #[derive(Debug)] @@ -504,18 +503,29 @@ impl TimeSlice { } } - #[allow(dead_code)] + /// Iterate this slice alone until its posteriors stop moving, returning + /// the number of iterations taken. + /// + /// Only used by tests: production convergence is driven across slices by + /// `History::converge`. + /// + /// Honours `self.convergence`; it previously hard-coded an epsilon and a + /// 20-iteration cap that matched neither `ConvergenceOptions` nor the + /// schedule default. + #[cfg(test)] pub(crate) fn iterate_to_convergence>( &mut self, agents: &CompetitorStore, ) -> usize { - let epsilon = 1e-6; - let iterations = 20; + use crate::{tuple_gt, tuple_max}; + + let epsilon = self.convergence.epsilon; + let max_iter = self.convergence.max_iter; let mut step = (f64::INFINITY, f64::INFINITY); let mut i = 0; - while tuple_gt(step, epsilon) && i < iterations { + while tuple_gt(step, epsilon) && i < max_iter { let old = self.posteriors(); self.iteration(0, agents); @@ -527,6 +537,10 @@ impl TimeSlice { }); i += 1; + + if !crate::step_is_finite(step) { + break; + } } i @@ -918,19 +932,24 @@ mod tests { let post = time_slice.posteriors(); + // These are convergence residuals, not exact values: by symmetry the + // true mean is 25.0 and the iteration approaches it from above. The + // previous expectation of 25.000003 was the residual after the + // hard-coded 20-iteration cap; honouring `ConvergenceOptions` runs to + // 30 and lands nearer the truth. assert_ulps_eq!( post[&a], - Gaussian::from_ms(25.000003, 3.880150), + Gaussian::from_ms(25.000001, 3.880150), epsilon = 1e-6 ); assert_ulps_eq!( post[&b], - Gaussian::from_ms(25.000003, 3.880150), + Gaussian::from_ms(25.000001, 3.880150), epsilon = 1e-6 ); assert_ulps_eq!( post[&c], - Gaussian::from_ms(25.000003, 3.880150), + Gaussian::from_ms(25.000001, 3.880150), epsilon = 1e-6 ); }