diff --git a/CLAUDE.md b/CLAUDE.md index 2c12113..83f2c32 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -80,11 +80,11 @@ History → TimeSlice[] → Event[] → Item[] `tau = mu/sigma²`). `Mul`/`Div` are the EP product/cavity: pure adds and subtracts. Variance-space ops (`Add`, `Sub`, `exclude`, `forget`) go through `from_mv`/`variance()` and take no square root. -- **`factor/`** — `TeamSumFactor`, `RankDiffFactor`, `TruncFactor` (ranked), - `MarginFactor` (scored), over a flat `VarStore`. `BuiltinFactor` dispatches - by enum rather than `dyn`. -- **`Schedule`** (`schedule.rs`) — drives factor propagation. `EpsilonOrMax` is - the only implementation. +- **`factor/`** — `TruncFactor` (ranked) and `MarginFactor` (scored) over a + flat `VarStore`. `Game::run_chain` drives them directly through a local + `DiffFactor` enum; there is no `Schedule` indirection and no generic `Factor` + trait. Both were removed once measurement showed nothing had ever used them + — see #42. - **`Competitor`** (`competitor.rs`) — per-history temporal state (`message`, `last_time`). **`Rating`** (`rating.rs`) — static config (prior, `beta`, drift). - **`storage/`** — `SkillStore` (per slice, `pub(crate)`) and `CompetitorStore` diff --git a/src/factor/margin.rs b/src/factor/margin.rs index dd23403..61b76d9 100644 --- a/src/factor/margin.rs +++ b/src/factor/margin.rs @@ -1,6 +1,6 @@ use crate::{ N_INF, - factor::{Factor, VarId, VarStore}, + factor::{VarId, VarStore}, gaussian::Gaussian, ln_pdf, }; @@ -55,12 +55,16 @@ impl MarginFactor { } } -impl Factor for MarginFactor { - fn propagate(&mut self, vars: &mut VarStore) -> (f64, f64) { +/// Undamped wrappers, used by this module's tests. Inference drives these +/// factors through `propagate_with_alpha` and reads the cached log evidence +/// directly, so these are not on any production path. +#[cfg(test)] +impl MarginFactor { + pub(crate) fn propagate(&mut self, vars: &mut VarStore) -> (f64, f64) { self.propagate_with_alpha(vars, 1.0) } - fn log_evidence(&self, _vars: &VarStore) -> f64 { + pub(crate) fn log_evidence(&self) -> f64 { self.log_evidence_cached.unwrap_or(0.0) } } @@ -146,7 +150,7 @@ mod tests { let diff = vars.alloc(Gaussian::from_ms(0.0, 6.0)); let mut f = MarginFactor::new(diff, 5.0, 1.0); f.propagate(&mut vars); - let logz = f.log_evidence(&vars); + let logz = f.log_evidence(); assert!((logz - (-3.062235327364623)).abs() < 1e-10); } diff --git a/src/factor/mod.rs b/src/factor/mod.rs index 04a2dc5..4578755 100644 --- a/src/factor/mod.rs +++ b/src/factor/mod.rs @@ -20,6 +20,8 @@ pub struct VarStore { } impl VarStore { + /// Test-only: inference allocates its store through `ScratchArena`. + #[cfg(test)] #[must_use] pub fn new() -> Self { Self::default() @@ -29,16 +31,13 @@ impl VarStore { self.marginals.clear(); } + /// Test-only, as `new`. + #[cfg(test)] #[must_use] pub fn len(&self) -> usize { self.marginals.len() } - #[must_use] - pub fn is_empty(&self) -> bool { - self.marginals.is_empty() - } - pub fn alloc(&mut self, init: Gaussian) -> VarId { let id = VarId(self.marginals.len() as u32); self.marginals.push(init); @@ -55,58 +54,7 @@ impl VarStore { } } -/// A factor in the EP graph. -/// -/// Factors hold their own outgoing messages and propagate them by reading -/// connected variable marginals from a `VarStore` and writing back updated -/// marginals. -pub trait Factor: Send + Sync { - /// Update outgoing messages and write back to the var store. - /// - /// Returns the max delta `(|Δmu|, |Δsigma|)` across writes this - /// propagation. Used by the `Schedule` to detect convergence. - fn propagate(&mut self, vars: &mut VarStore) -> (f64, f64); - - /// Optional log-evidence contribution. Default 0.0 (no contribution). - fn log_evidence(&self, _vars: &VarStore) -> f64 { - 0.0 - } -} - -/// Enum dispatcher for the built-in factor types. -/// -/// Using an enum instead of `Box` keeps factor data inline and -/// avoids virtual-call overhead in the hot inference loop. -#[derive(Debug)] -pub enum BuiltinFactor { - TeamSum(team_sum::TeamSumFactor), - RankDiff(rank_diff::RankDiffFactor), - Trunc(trunc::TruncFactor), - Margin(margin::MarginFactor), -} - -impl Factor for BuiltinFactor { - fn propagate(&mut self, vars: &mut VarStore) -> (f64, f64) { - match self { - Self::TeamSum(f) => f.propagate(vars), - Self::RankDiff(f) => f.propagate(vars), - Self::Trunc(f) => f.propagate(vars), - Self::Margin(f) => f.propagate(vars), - } - } - - fn log_evidence(&self, vars: &VarStore) -> f64 { - match self { - Self::Trunc(f) => f.log_evidence(vars), - Self::Margin(f) => f.log_evidence(vars), - Self::TeamSum(_) | Self::RankDiff(_) => 0.0, - } - } -} - pub mod margin; -pub mod rank_diff; -pub mod team_sum; pub mod trunc; #[cfg(test)] @@ -153,20 +101,4 @@ mod tests { assert_eq!(store.len(), 0); assert_eq!(store.marginals.capacity(), cap); } - - #[test] - fn builtin_factor_dispatches_to_margin() { - use super::margin::MarginFactor; - let mut vars = VarStore::new(); - let diff = vars.alloc(Gaussian::from_ms(0.0, 6.0)); - let mut f = BuiltinFactor::Margin(MarginFactor::new(diff, 5.0, 1.0)); - - f.propagate(&mut vars); - - let result = vars.get(diff); - assert!((result.mu() - 4.864864864864865).abs() < 1e-12); - - let logz = f.log_evidence(&vars); - assert!((logz - (-3.062235327364623)).abs() < 1e-10); - } } diff --git a/src/factor/rank_diff.rs b/src/factor/rank_diff.rs deleted file mode 100644 index d36f568..0000000 --- a/src/factor/rank_diff.rs +++ /dev/null @@ -1,95 +0,0 @@ -use crate::factor::{Factor, VarId, VarStore}; - -/// Maintains the constraint `diff = team_a - team_b` between three vars. -/// -/// On each propagation: -/// - Reads marginals at `team_a` and `team_b` (which already incorporate any -/// incoming messages from neighboring factors). -/// - Computes `new_diff = team_a - team_b` (variance addition; see `Gaussian::Sub`). -/// - Writes the new marginal to `diff`. -/// - Returns the delta against the previous diff value. -/// -/// This factor does NOT store an outgoing message; the diff variable is -/// effectively replaced on each propagation. The `TruncFactor` on the same diff -/// var holds the EP-divide message that produces the cavity. -#[derive(Debug)] -pub struct RankDiffFactor { - pub team_a: VarId, - pub team_b: VarId, - pub diff: VarId, -} - -impl Factor for RankDiffFactor { - fn propagate(&mut self, vars: &mut VarStore) -> (f64, f64) { - let a = vars.get(self.team_a); - let b = vars.get(self.team_b); - let new_diff = a - b; - let old = vars.get(self.diff); - vars.set(self.diff, new_diff); - old.delta(new_diff) - } -} - -#[cfg(test)] -mod tests { - use super::*; - use crate::{N_INF, gaussian::Gaussian}; - - #[test] - fn diff_of_two_known_gaussians() { - let mut vars = VarStore::new(); - let team_a = vars.alloc(Gaussian::from_ms(25.0, 3.0)); - let team_b = vars.alloc(Gaussian::from_ms(20.0, 4.0)); - let diff = vars.alloc(N_INF); - - let mut f = RankDiffFactor { - team_a, - team_b, - diff, - }; - f.propagate(&mut vars); - - let result = vars.get(diff); - // mu = 25 - 20 = 5; var = 9 + 16 = 25; sigma = 5 - assert!((result.mu() - 5.0).abs() < 1e-12); - assert!((result.sigma() - 5.0).abs() < 1e-12); - } - - #[test] - fn delta_zero_on_repeat() { - let mut vars = VarStore::new(); - let team_a = vars.alloc(Gaussian::from_ms(10.0, 2.0)); - let team_b = vars.alloc(Gaussian::from_ms(8.0, 1.0)); - let diff = vars.alloc(N_INF); - - let mut f = RankDiffFactor { - team_a, - team_b, - diff, - }; - f.propagate(&mut vars); - let (dmu, dsig) = f.propagate(&mut vars); - assert!(dmu < 1e-12); - assert!(dsig < 1e-12); - } - - #[test] - fn delta_reflects_team_change() { - let mut vars = VarStore::new(); - let team_a = vars.alloc(Gaussian::from_ms(10.0, 1.0)); - let team_b = vars.alloc(Gaussian::from_ms(0.0, 1.0)); - let diff = vars.alloc(N_INF); - - let mut f = RankDiffFactor { - team_a, - team_b, - diff, - }; - f.propagate(&mut vars); - - // change team_a, repropagate; delta should be positive - vars.set(team_a, Gaussian::from_ms(15.0, 1.0)); - let (dmu, _dsig) = f.propagate(&mut vars); - assert!(dmu > 4.0, "expected ~5 delta, got {}", dmu); - } -} diff --git a/src/factor/team_sum.rs b/src/factor/team_sum.rs deleted file mode 100644 index a110141..0000000 --- a/src/factor/team_sum.rs +++ /dev/null @@ -1,98 +0,0 @@ -use crate::{ - N00, - factor::{Factor, VarId, VarStore}, - gaussian::Gaussian, -}; - -/// Computes the weighted sum of player performances into a team-perf var. -/// -/// Inputs are pre-computed player performance Gaussians (i.e., rating priors -/// already with beta² noise added via `Rating::performance()`). The factor -/// runs once per game and writes the weighted sum to the output var. -#[derive(Debug)] -pub struct TeamSumFactor { - pub inputs: Vec<(Gaussian, f64)>, - pub out: VarId, -} - -impl Factor for TeamSumFactor { - fn propagate(&mut self, vars: &mut VarStore) -> (f64, f64) { - let perf = self.inputs.iter().fold(N00, |acc, (g, w)| acc + (*g * *w)); - let old = vars.get(self.out); - vars.set(self.out, perf); - old.delta(perf) - } -} - -#[cfg(test)] -mod tests { - use super::*; - use crate::N_INF; - - #[test] - fn single_player_unit_weight() { - let mut vars = VarStore::new(); - let out = vars.alloc(N_INF); - let g = Gaussian::from_ms(25.0, 5.0); - let mut f = TeamSumFactor { - inputs: vec![(g, 1.0)], - out, - }; - - f.propagate(&mut vars); - let result = vars.get(out); - assert!((result.mu() - 25.0).abs() < 1e-12); - assert!((result.sigma() - 5.0).abs() < 1e-12); - } - - #[test] - fn two_players_summed() { - let mut vars = VarStore::new(); - let out = vars.alloc(N_INF); - let g1 = Gaussian::from_ms(20.0, 3.0); - let g2 = Gaussian::from_ms(30.0, 4.0); - let mut f = TeamSumFactor { - inputs: vec![(g1, 1.0), (g2, 1.0)], - out, - }; - - f.propagate(&mut vars); - let result = vars.get(out); - // sum: mu = 20 + 30 = 50, var = 9 + 16 = 25, sigma = 5 - assert!((result.mu() - 50.0).abs() < 1e-12); - assert!((result.sigma() - 5.0).abs() < 1e-12); - } - - #[test] - fn weighted_inputs() { - let mut vars = VarStore::new(); - let out = vars.alloc(N_INF); - let g = Gaussian::from_ms(10.0, 2.0); - let mut f = TeamSumFactor { - inputs: vec![(g, 2.0)], - out, - }; - - f.propagate(&mut vars); - let result = vars.get(out); - // g * 2.0: mu = 10*2 = 20, sigma = 2*2 = 4 - assert!((result.mu() - 20.0).abs() < 1e-12); - assert!((result.sigma() - 4.0).abs() < 1e-12); - } - - #[test] - fn delta_is_zero_on_repeat_propagate() { - let mut vars = VarStore::new(); - let out = vars.alloc(N_INF); - let g = Gaussian::from_ms(5.0, 1.0); - let mut f = TeamSumFactor { - inputs: vec![(g, 1.0)], - out, - }; - - f.propagate(&mut vars); - let (dmu, dsig) = f.propagate(&mut vars); - assert!(dmu < 1e-12, "expected ~0 delta on repeat, got {}", dmu); - assert!(dsig < 1e-12); - } -} diff --git a/src/factor/trunc.rs b/src/factor/trunc.rs index 49ef63b..3105dc6 100644 --- a/src/factor/trunc.rs +++ b/src/factor/trunc.rs @@ -1,6 +1,6 @@ use crate::{ N_INF, approx, - factor::{Factor, VarId, VarStore}, + factor::{VarId, VarStore}, gaussian::Gaussian, ln_interval, ln_sf, }; @@ -63,14 +63,14 @@ impl TruncFactor { } } -impl Factor for TruncFactor { - fn propagate(&mut self, vars: &mut VarStore) -> (f64, f64) { +/// Undamped wrappers, used by this module's tests. Inference drives these +/// factors through `propagate_with_alpha` and reads the cached log evidence +/// directly, so these are not on any production path. +#[cfg(test)] +impl TruncFactor { + pub(crate) fn propagate(&mut self, vars: &mut VarStore) -> (f64, f64) { self.propagate_with_alpha(vars, 1.0) } - - fn log_evidence(&self, _vars: &VarStore) -> f64 { - self.log_evidence_cached.unwrap_or(0.0) - } } /// `ln P(diff > margin)` for a win, `ln P(|diff| < margin)` for a tie. diff --git a/src/game.rs b/src/game.rs index c9b5be3..cc58c4a 100644 --- a/src/game.rs +++ b/src/game.rs @@ -568,15 +568,6 @@ impl> Game<'_, T, D> { let team_refs: Vec<&[Rating]> = teams.iter().map(|t| t.as_slice()).collect(); Self::ranked(&team_refs, outcome, options) } - - #[doc(hidden)] - pub fn custom( - factors: &mut [crate::graph::BuiltinFactor], - vars: &mut crate::graph::VarStore, - schedule: &S, - ) -> crate::graph::ScheduleReport { - schedule.run(factors, vars) - } } #[cfg(test)] diff --git a/src/graph.rs b/src/graph.rs deleted file mode 100644 index abd6395..0000000 --- a/src/graph.rs +++ /dev/null @@ -1,20 +0,0 @@ -//! Factor-graph public API. -//! -//! Named `graph` rather than `factors` because the private implementation -//! module beside it is `factor`: two module paths differing by one character, -//! one public and one not, was a standing invitation to import the wrong one. -//! -//! The factor types, `VarStore` and the `Schedule` trait are public so custom -//! schedules can be written against them. -//! -//! Building a factor graph by hand goes through `Game::custom`, which is -//! deliberately `#[doc(hidden)]`: it works, but its signature is not yet -//! considered stable API and so is not listed in these docs. - -pub use crate::{ - factor::{ - BuiltinFactor, Factor, VarId, VarStore, margin::MarginFactor, rank_diff::RankDiffFactor, - team_sum::TeamSumFactor, trunc::TruncFactor, - }, - schedule::{EpsilonOrMax, Schedule, ScheduleReport}, -}; diff --git a/src/lib.rs b/src/lib.rs index 715975f..f58c41b 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -121,7 +121,6 @@ mod event_builder; pub(crate) mod factor; mod game; pub mod gaussian; -pub mod graph; mod history; mod joint; mod key_table; @@ -131,7 +130,6 @@ mod outcome; mod predict; pub(crate) mod quadrature; mod rating; -pub(crate) mod schedule; pub mod storage; pub use acquisition::expected_information_gain; @@ -150,7 +148,6 @@ pub use observer::{NullObserver, Observer}; pub use outcome::Outcome; pub use predict::Prediction; pub use rating::Rating; -pub use schedule::ScheduleReport; pub use time::{Time, Untimed}; pub const BETA: f64 = 1.0; diff --git a/src/outcome.rs b/src/outcome.rs index 3200b75..526f595 100644 --- a/src/outcome.rs +++ b/src/outcome.rs @@ -34,12 +34,41 @@ impl Outcome { /// /// # Panics /// - /// Panics if `winner >= n`. + /// Panics if `winner >= n`. Use [`Outcome::try_winner`] when the index + /// comes from data rather than a literal. + /// + /// This is the one constructor here that validates, and deliberately so. + /// Its siblings build freely and let ingestion reject what it cannot use, + /// which works because a malformed rank vector stays recognisable. An + /// out-of-range winner does not: `winner(5, 2)` would produce ranks + /// `[1, 1]`, an all-tied draw that ingestion accepts without complaint when + /// `p_draw > 0`. Asking "team 5 won" and silently getting "everyone drew" + /// is exactly the class of quiet wrong answer this crate keeps removing, so + /// the check happens here where the mistake is. #[must_use] pub fn winner(winner: u32, n: u32) -> Self { - assert!(winner < n, "winner index {winner} out of range 0..{n}"); + Self::try_winner(winner, n) + .unwrap_or_else(|_| panic!("winner index {winner} out of range 0..{n}")) + } + + /// `n`-team outcome where team `winner` won, or an error if `winner` is not + /// a valid team index. + /// + /// The fallible form of [`Outcome::winner`], for when the index is computed + /// or parsed rather than written literally. + /// + /// # Errors + /// + /// `InvalidParameter` if `winner >= n`. + pub fn try_winner(winner: u32, n: u32) -> Result { + if winner >= n { + return Err(crate::InferenceError::InvalidParameter { + name: "winner", + value: f64::from(winner), + }); + } let ranks: SmallVec<[u32; 4]> = (0..n).map(|i| if i == winner { 0 } else { 1 }).collect(); - Self::Ranked(ranks) + Ok(Self::Ranked(ranks)) } /// All `n` teams tied. diff --git a/src/schedule.rs b/src/schedule.rs deleted file mode 100644 index 0614907..0000000 --- a/src/schedule.rs +++ /dev/null @@ -1,152 +0,0 @@ -//! Schedule trait and built-in implementations. -//! -//! A schedule drives factor propagation to convergence. The default -//! `EpsilonOrMax` performs one `TeamSum` sweep (setup) then alternating -//! forward/backward sweeps over the iterating factors until the max -//! delta drops below epsilon or `max` iterations is reached. - -use crate::factor::{BuiltinFactor, Factor, VarStore}; - -/// Result returned by a `Schedule::run` call. -#[derive(Debug, Clone, Copy)] -pub struct ScheduleReport { - pub iterations: usize, - pub final_step: (f64, f64), - pub converged: bool, -} - -/// Drives factor propagation to convergence. -pub trait Schedule: Send + Sync { - fn run(&self, factors: &mut [BuiltinFactor], vars: &mut VarStore) -> ScheduleReport; -} - -/// Default schedule: sweep forward then backward until step ≤ eps or iter == max. -/// -/// Matches the existing `Game::likelihoods` loop bit-for-bit when given the -/// same factor layout (`TeamSums` first, then alternating RankDiff/Trunc pairs). -#[derive(Debug, Clone, Copy)] -pub struct EpsilonOrMax { - pub eps: f64, - pub max: usize, -} - -impl Default for EpsilonOrMax { - fn default() -> Self { - // 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, - } - } -} - -impl Schedule for EpsilonOrMax { - fn run(&self, factors: &mut [BuiltinFactor], vars: &mut VarStore) -> ScheduleReport { - // Partition: leading run of TeamSum factors run exactly once (setup). - let n_setup = factors - .iter() - .position(|f| !matches!(f, BuiltinFactor::TeamSum(_))) - .unwrap_or(factors.len()); - - for f in factors[..n_setup].iter_mut() { - f.propagate(vars); - } - - let mut iterations = 0; - // 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); - - // Forward sweep over iterating factors. - for f in factors[n_setup..].iter_mut() { - let d = f.propagate(vars); - step.0 = step.0.max(d.0); - step.1 = step.1.max(d.1); - } - - // Backward sweep. - for f in factors[n_setup..].iter_mut().rev() { - let d = f.propagate(vars); - step.0 = step.0.max(d.0); - step.1 = step.1.max(d.1); - } - - iterations += 1; - final_step = step; - - if step.0 <= self.eps && step.1 <= self.eps { - converged = true; - break; - } - } - } - - ScheduleReport { - iterations, - final_step, - converged, - } - } -} - -#[cfg(test)] -mod tests { - use super::*; - use crate::{N_INF, factor::team_sum::TeamSumFactor, gaussian::Gaussian}; - - #[test] - fn schedule_runs_setup_factors_once() { - // Single TeamSum factor; schedule should propagate it exactly once and report 0 iterations. - let mut vars = VarStore::new(); - let out = vars.alloc(N_INF); - let mut factors = vec![BuiltinFactor::TeamSum(TeamSumFactor { - inputs: vec![(Gaussian::from_ms(5.0, 1.0), 1.0)], - out, - })]; - let schedule = EpsilonOrMax::default(); - let report = schedule.run(&mut factors, &mut vars); - assert_eq!(report.iterations, 0); - // The team-perf var should hold the sum. - let result = vars.get(out); - assert!((result.mu() - 5.0).abs() < 1e-12); - } - - #[test] - fn report_marks_converged_when_no_iterating_factors() { - // 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 { - inputs: vec![(Gaussian::from_ms(0.0, 1.0), 1.0)], - out, - })]; - 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); - } -}