From 507894dae7999d80d6274bb92b9fa6156a596eaa Mon Sep 17 00:00:00 2001 From: Anders Olsson Date: Mon, 7 Sep 2026 14:57:39 +0200 Subject: [PATCH] refactor!: close the remaining API gaps from #21 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three unrelated small defects, all requiring signature changes: - `Game::one_v_one` hardcoded `GameOptions::default()`, so a 1v1 could never set `p_draw` or convergence options — and a drawn 1v1 was therefore unreachable through it, since the default `p_draw` is zero. It now takes `&GameOptions` like every other constructor. - `Observer::on_batch_processed` was declared on the trait and never called from anywhere: implementors wired up a callback that could not fire. It is now called after each slice sweep, and renamed `on_slice_processed` to match the vocabulary the codebase adopted in T2 — the unit of work is a `TimeSlice`, not a batch. A slice is swept once travelling backward and once forward, so a multi-slice history fires it twice per slice per iteration; the doc comment says so. - `pub mod factors` sat beside `pub(crate) mod factor`, two module paths differing by one character with only one of them importable. The public facade is now `graph`. Tests cover each as a behaviour rather than a compile check: a drawn 1v1 succeeds only when p_draw is supplied, and the observer tests fail if any callback stops firing. BREAKING CHANGE: `Game::one_v_one` takes a fourth `&GameOptions` argument; `Observer::on_batch_processed` is renamed `on_slice_processed`; the `factors` module is renamed `graph`. Closes #21 Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_011hcFjNDmHXZF8URGLku5zZ --- src/game.rs | 20 ++++--- src/{factors.rs => graph.rs} | 4 ++ src/history.rs | 15 +++++ src/lib.rs | 2 +- src/observer.rs | 10 +++- tests/equivalence.rs | 3 +- tests/game.rs | 45 ++++++++++++++- tests/observer.rs | 105 +++++++++++++++++++++++++++++++++++ 8 files changed, 190 insertions(+), 14 deletions(-) rename src/{factors.rs => graph.rs} (72%) create mode 100644 tests/observer.rs diff --git a/src/game.rs b/src/game.rs index bc69584..3c7ca99 100644 --- a/src/game.rs +++ b/src/game.rs @@ -532,18 +532,20 @@ impl> Game<'_, T, D> { )) } + /// Convenience wrapper over [`Game::ranked`] for two single-player teams. + /// /// # Errors /// - /// Delegates to [`Game::ranked`] with default options, so it returns the - /// same errors — in practice `WrongOutcomeKind` for a non-ranked outcome, - /// or `TieWithoutDrawProbability` for a draw, since the default `p_draw` - /// applies rather than one you chose. + /// Delegates to [`Game::ranked`], so it returns the same errors — in + /// practice `WrongOutcomeKind` for a non-ranked outcome, or + /// `TieWithoutDrawProbability` for a draw when `options.p_draw` is zero. pub fn one_v_one( a: &Rating, b: &Rating, outcome: crate::Outcome, + options: &GameOptions, ) -> Result<(Gaussian, Gaussian), crate::InferenceError> { - let game = Self::ranked(&[&[*a], &[*b]], outcome, &GameOptions::default())?; + let game = Self::ranked(&[&[*a], &[*b]], outcome, options)?; let post = game.posteriors(); Ok((post[0][0], post[1][0])) } @@ -563,11 +565,11 @@ impl> Game<'_, T, D> { } #[doc(hidden)] - pub fn custom( - factors: &mut [crate::factors::BuiltinFactor], - vars: &mut crate::factors::VarStore, + pub fn custom( + factors: &mut [crate::graph::BuiltinFactor], + vars: &mut crate::graph::VarStore, schedule: &S, - ) -> crate::factors::ScheduleReport { + ) -> crate::graph::ScheduleReport { schedule.run(factors, vars) } } diff --git a/src/factors.rs b/src/graph.rs similarity index 72% rename from src/factors.rs rename to src/graph.rs index 4a945e7..abd6395 100644 --- a/src/factors.rs +++ b/src/graph.rs @@ -1,5 +1,9 @@ //! 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. //! diff --git a/src/history.rs b/src/history.rs index 1a29bbf..6aa74c7 100644 --- a/src/history.rs +++ b/src/history.rs @@ -261,6 +261,11 @@ impl, O: Observer, K: Eq + Hash + Clone> History, O: Observer, K: Eq + Hash + Clone> History, O: Observer, K: Eq + Hash + Clone> History: Send + Sync { /// Called after each convergence iteration across the whole history. fn on_iteration_end(&self, _iter: usize, _max_step: (f64, f64)) {} - /// Called after each time slice is processed within an iteration. - fn on_batch_processed(&self, _time: &T, _slice_idx: usize, _n_events: usize) {} + /// Called after each time slice is swept within an iteration. + /// + /// A convergence iteration sweeps every slice twice — once travelling + /// backward through the history and once forward — so a multi-slice + /// history fires this twice per slice per iteration. A single-slice + /// history is swept once and fires once. + fn on_slice_processed(&self, _time: &T, _slice_idx: usize, _n_events: usize) {} /// Called once when convergence completes (or max iters is reached). fn on_converged(&self, _iters: usize, _final_step: (f64, f64), _converged: bool) {} @@ -35,6 +40,7 @@ mod tests { fn null_observer_compiles_for_i64() { let o = NullObserver; >::on_iteration_end(&o, 1, (0.0, 0.0)); + >::on_slice_processed(&o, &7, 0, 3); >::on_converged(&o, 5, (1e-6, 1e-6), true); } diff --git a/tests/equivalence.rs b/tests/equivalence.rs index 9c7c75f..cabd076 100644 --- a/tests/equivalence.rs +++ b/tests/equivalence.rs @@ -19,7 +19,8 @@ fn ts_rating(mu: f64, sigma: f64, beta: f64, gamma: f64) -> R { fn game_1v1_golden_matches_historical() { let a = ts_rating(25.0, 25.0 / 3.0, 25.0 / 6.0, 25.0 / 300.0); let b = ts_rating(25.0, 25.0 / 3.0, 25.0 / 6.0, 25.0 / 300.0); - let (a_post, b_post) = Game::::one_v_one(&a, &b, Outcome::winner(0, 2)).unwrap(); + let (a_post, b_post) = + Game::::one_v_one(&a, &b, Outcome::winner(0, 2), &GameOptions::default()).unwrap(); // Historical golden from pre-T2 test_1vs1 (team 0 wins): assert_ulps_eq!( a_post, diff --git a/tests/game.rs b/tests/game.rs index 5fc84ce..7fe7140 100644 --- a/tests/game.rs +++ b/tests/game.rs @@ -32,7 +32,8 @@ fn game_ranked_1v1_golden() { fn game_one_v_one_shortcut() { let a = default_rating(); let b = default_rating(); - let (a_post, b_post) = Game::::one_v_one(&a, &b, Outcome::winner(0, 2)).unwrap(); + let (a_post, b_post) = + Game::::one_v_one(&a, &b, Outcome::winner(0, 2), &GameOptions::default()).unwrap(); assert!(a_post.mu() > 25.0); assert!(b_post.mu() < 25.0); } @@ -95,3 +96,45 @@ fn game_log_evidence_is_finite() { assert!(g.log_evidence().is_finite()); assert!(g.log_evidence() < 0.0); } + +/// `one_v_one` used to hardcode `GameOptions::default()`, so a 1v1 could +/// never set `p_draw` and a drawn 1v1 was unreachable through it. +#[test] +fn one_v_one_honours_the_draw_probability_it_is_given() { + let a = default_rating(); + let b = default_rating(); + + // Default options still reject a draw, because the default p_draw is zero. + let err = Game::::one_v_one(&a, &b, Outcome::draw(2), &GameOptions::default()) + .expect_err("a draw needs a positive p_draw"); + assert!(matches!( + err, + InferenceError::TieWithoutDrawProbability { .. } + )); + + // With a draw probability supplied it succeeds — which was impossible + // before the signature took options. + let options = GameOptions { + p_draw: 0.25, + ..GameOptions::default() + }; + let (a_post, b_post) = Game::::one_v_one(&a, &b, Outcome::draw(2), &options) + .expect("a draw is representable once p_draw is positive"); + + // A symmetric draw leaves the means alone and sharpens both sides. + assert!((a_post.mu() - b_post.mu()).abs() < 1e-9); + assert!(a_post.sigma() < 25.0 / 3.0); +} + +/// Convergence options reach the 1v1 path too, not just `p_draw`. +#[test] +fn one_v_one_honours_convergence_options() { + let a = default_rating(); + let b = default_rating(); + let options = GameOptions { + convergence: ConvergenceOptions::default(), + ..GameOptions::default() + }; + let (a_post, _) = Game::::one_v_one(&a, &b, Outcome::winner(0, 2), &options).unwrap(); + assert!(a_post.mu() > 25.0); +} diff --git a/tests/observer.rs b/tests/observer.rs new file mode 100644 index 0000000..94d37fb --- /dev/null +++ b/tests/observer.rs @@ -0,0 +1,105 @@ +//! `Observer` callbacks must actually fire. +//! +//! `on_slice_processed` (formerly `on_batch_processed`) was declared on the +//! trait and never called from anywhere, so implementors wired up a callback +//! that could not run. These tests exist so that cannot silently recur. + +use std::sync::{Arc, Mutex}; + +use trueskill_tt::{History, Observer}; + +/// `History` takes its observer by value and never hands it back, so a test +/// that wants to read what was recorded shares the storage rather than the +/// observer: the handles are cloned, the buffers are not. +#[derive(Clone, Default)] +struct Recorder { + iterations: Arc>>, + slices: Arc>>, + converged: Arc>>, +} + +impl Observer for Recorder { + fn on_iteration_end(&self, iter: usize, _max_step: (f64, f64)) { + self.iterations.lock().unwrap().push(iter); + } + + fn on_slice_processed(&self, time: &i64, slice_idx: usize, n_events: usize) { + self.slices + .lock() + .unwrap() + .push((*time, slice_idx, n_events)); + } + + fn on_converged(&self, iters: usize, _final_step: (f64, f64), converged: bool) { + self.converged.lock().unwrap().push((iters, converged)); + } +} + +#[test] +fn every_observer_callback_fires() { + let recorder = Recorder::default(); + let mut h = History::builder().observer(recorder.clone()).build(); + + h.record_winner(&"a", &"b", 1).unwrap(); + h.record_winner(&"b", &"c", 2).unwrap(); + h.record_winner(&"c", &"a", 3).unwrap(); + h.converge().unwrap(); + + assert!( + !recorder.iterations.lock().unwrap().is_empty(), + "on_iteration_end never fired" + ); + assert!( + !recorder.converged.lock().unwrap().is_empty(), + "on_converged never fired" + ); + assert!( + !recorder.slices.lock().unwrap().is_empty(), + "on_slice_processed never fired — the defect this test exists for" + ); +} + +#[test] +fn slice_callbacks_report_the_slice_they_swept() { + let recorder = Recorder::default(); + let mut h = History::builder().observer(recorder.clone()).build(); + + h.record_winner(&"a", &"b", 10).unwrap(); + h.record_winner(&"a", &"b", 20).unwrap(); + h.converge().unwrap(); + + let slices = recorder.slices.lock().unwrap(); + + // Only the times actually in the history, and each with its own events. + for &(time, idx, events) in slices.iter() { + assert!(time == 10 || time == 20, "unexpected slice time {time}"); + assert!(idx < 2, "slice index {idx} out of range"); + assert_eq!(events, 1, "each slice holds exactly one event"); + } + + // Both slices must be reported, not just one end of the sweep. + assert!( + slices.iter().any(|&(t, ..)| t == 10), + "slice 10 never reported" + ); + assert!( + slices.iter().any(|&(t, ..)| t == 20), + "slice 20 never reported" + ); +} + +#[test] +fn a_single_slice_history_still_reports_its_sweep() { + let recorder = Recorder::default(); + let mut h = History::builder().observer(recorder.clone()).build(); + + h.record_winner(&"a", &"b", 1).unwrap(); + h.converge().unwrap(); + + let slices = recorder.slices.lock().unwrap(); + assert!( + !slices.is_empty(), + "the single-slice path must report its sweep too" + ); + assert!(slices.iter().all(|&(t, idx, _)| t == 1 && idx == 0)); +}