From 4472d98b567dab82ee5259623565cbeb3521b3aa Mon Sep 17 00:00:00 2001 From: Anders Olsson Date: Wed, 9 Sep 2026 20:34:18 +0200 Subject: [PATCH] refactor!: un-export six types that no caller could reach MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `TimeSlice`, `EventKind`, `KeyTable`, `CompetitorStore`, `Competitor` and the `storage` module were all public and none was obtainable from a `History` — `time_slices`, `agents` and `keys` are all private or `pub(crate)`. `TimeSlice` was the worst: `new`, `add_events`, `iteration`, `get_composition` and `get_results` were `pub` on a type you could only build standalone and never feed back into anything. Their sole consumer outside `src/` was `benches/batch.rs`, so a benchmark was dictating six public types. It is rewritten against the public API: a single-slice history's `converge` calls exactly the same per-slice sweep, so capping at one iteration measures the same code path. `N01` had zero references in the entire repository, including inside the crate; removed. `N00` and `N_INF` are EP identities (`Add` and `Mul`) and are now `pub(crate)` — a user reaching for `N_INF` as "an unknown competitor's prior" would get an improper distribution whose `mu()` silently reports 0.0. Adds the accessors their absence forced people around, from #70: `competitors()`, `competitor_count()` and `event_count()` (`size` had no accessor at all). Answering "who is best" previously meant materialising every competitor's full smoothed curve to read the last point of each. `KeyTable::keys` now iterates the dense reverse table rather than the forward `HashMap`, so `competitors()` yields insertion order rather than per-process hash order — the same hazard as #62, caught before it could reach a caller building a standings table. Two `CompetitorStore` methods (`is_empty`, `iter_mut`) had no callers anywhere and are gone; four more are now `#[cfg(test)]`, which is what they always were in practice. Worth recording a mistake: I first deleted `get_composition`/`get_results` on the strength of a "never used" warning, and the build broke — the warning came from the plain-lib target, where `#[cfg(test)]` callers in history.rs are not compiled. A dead-code warning from one target is not evidence about the others. BREAKING CHANGE: `TimeSlice`, `EventKind`, `KeyTable`, `CompetitorStore`, `Competitor`, the `storage` module, `N01`, `N00` and `N_INF` are no longer public. Refs #73, #70 Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_011hcFjNDmHXZF8URGLku5zZ --- benches/batch.rs | 82 ++++++++++++++++++--------------- src/history.rs | 33 +++++++++++++ src/key_table.rs | 15 +++--- src/lib.rs | 10 ++-- src/storage/competitor_store.rs | 17 ++----- src/time_slice.rs | 6 ++- 6 files changed, 99 insertions(+), 64 deletions(-) diff --git a/benches/batch.rs b/benches/batch.rs index 20bd6c7..b84f653 100644 --- a/benches/batch.rs +++ b/benches/batch.rs @@ -1,49 +1,57 @@ +//! One slice's event sweep. +//! +//! Written against the public API rather than against `TimeSlice` directly. +//! It used to reach for `TimeSlice`, `KeyTable`, `CompetitorStore`, +//! `Competitor` and `EventKind`, and was the *only* thing outside `src/` +//! that did — so a benchmark was dictating five public types that no test, +//! example or consumer could otherwise obtain. +//! +//! A single-slice history's `converge` calls exactly the same per-slice sweep, +//! so capping at one iteration measures the same code path. + use criterion::{Criterion, criterion_group, criterion_main}; +use smallvec::smallvec; use trueskill_tt::{ - BETA, Competitor, ConvergenceOptions, EventKind, GAMMA, KeyTable, MU, P_DRAW, Rating, SIGMA, - TimeSlice, drift::ConstantDrift, gaussian::Gaussian, storage::CompetitorStore, + ConstantDrift, ConvergenceOptions, Event, History, Member, Outcome, Team, }; fn criterion_benchmark(criterion: &mut Criterion) { - let mut index_map = KeyTable::new(); + let build = || { + let mut h = History::builder() + .convergence(ConvergenceOptions { + max_iter: 1, + epsilon: 0.0, + alpha: 1.0, + }) + .drift(ConstantDrift::new(0.0)) + .build(); - let a = index_map.get_or_create("a"); - let b = index_map.get_or_create("b"); - let c = index_map.get_or_create("c"); + // 100 events, all at one time, so the history has a single slice. + let events: Vec> = (0..100) + .map(|_| Event { + time: 1, + teams: smallvec![ + Team::with_members([Member::new("a")]), + Team::with_members([Member::new("b")]), + ], + outcome: Outcome::winner(0, 2), + }) + .collect(); + h.add_events(events).expect("fixture ingests"); + h + }; - let mut agents: CompetitorStore = CompetitorStore::new(); - - for agent in [a, b, c] { - agents.insert( - agent, - Competitor { - rating: Rating::new( - Gaussian::from_ms(MU, SIGMA), - BETA, - ConstantDrift::new(GAMMA), - ), - ..Default::default() + criterion.bench_function("slice_sweep_100_events", |b| { + b.iter_batched( + build, + |mut h| { + // `converge_partial`, not `converge`: one iteration is + // deliberately short of convergence and `converge` reports that + // as an error. + let _ = h.converge_partial(); }, + criterion::BatchSize::SmallInput, ); - } - - let mut composition = Vec::new(); - let mut results = Vec::new(); - let mut weights = Vec::new(); - - for _ in 0..100 { - composition.push(vec![vec![a], vec![b]]); - results.push(vec![1.0, 0.0]); - weights.push(vec![vec![1.0], vec![1.0]]); - } - - let kinds = vec![EventKind::Ranked; composition.len()]; - - let mut time_slice = TimeSlice::new(1, P_DRAW, ConvergenceOptions::default()); - time_slice.add_events(composition, Some(results), Some(weights), kinds, &agents); - - criterion.bench_function("Batch::iteration", |b| { - b.iter(|| time_slice.iteration(0, &agents)) }); } diff --git a/src/history.rs b/src/history.rs index b2ae468..98fbc28 100644 --- a/src/history.rs +++ b/src/history.rs @@ -546,6 +546,39 @@ impl, O: Observer, K: Eq + Hash + Clone> History = h.competitors().copied().collect(); + /// assert_eq!(names, ["alice", "bob"]); + /// # Ok::<(), trueskill_tt::InferenceError>(()) + /// ``` + pub fn competitors(&self) -> impl ExactSizeIterator { + self.keys.keys() + } + + /// How many competitors the history knows. + #[must_use] + pub fn competitor_count(&self) -> usize { + self.keys.len() + } + + /// How many events have been ingested. + #[must_use] + pub fn event_count(&self) -> usize { + self.size + } + /// Learning curves for all competitors, keyed by their user-facing key. pub fn learning_curves(&self) -> HashMap> { #[cfg(feature = "rayon")] diff --git a/src/key_table.rs b/src/key_table.rs index 40953d6..b06e526 100644 --- a/src/key_table.rs +++ b/src/key_table.rs @@ -60,19 +60,20 @@ where self.reverse.get(idx.0) } - pub fn keys(&self) -> impl Iterator { - self.forward.keys() + /// Every key, in the order they were first interned. + /// + /// Iterates the dense reverse table rather than the forward `HashMap`. + /// Rust seeds its default hasher per process, so a `HashMap` walk yields a + /// different order on every run — which is fine for membership but not for + /// anything a caller might sum, sort or print. + pub fn keys(&self) -> impl ExactSizeIterator { + self.reverse.iter() } #[must_use] pub fn len(&self) -> usize { self.reverse.len() } - - #[must_use] - pub fn is_empty(&self) -> bool { - self.reverse.is_empty() - } } impl Default for KeyTable diff --git a/src/lib.rs b/src/lib.rs index 1feca81..7927e63 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -109,7 +109,6 @@ mod approx; pub(crate) mod arena; mod time; mod time_slice; -pub use time_slice::{EventKind, TimeSlice}; mod acquisition; mod color_group; mod competitor; @@ -130,10 +129,9 @@ mod outcome; mod predict; pub(crate) mod quadrature; mod rating; -pub mod storage; +pub(crate) mod storage; pub use acquisition::expected_information_gain; -pub use competitor::Competitor; pub use convergence::{ConvergenceOptions, ConvergenceReport}; pub use drift::{ConstantDrift, Drift}; pub use error::{InferenceError, UnknownKeys}; @@ -142,7 +140,6 @@ pub use event_builder::EventBuilder; pub use game::{Game, GameOptions, OwnedGame}; pub use gaussian::Gaussian; pub use history::{History, HistoryBuilder, Joint}; -pub use key_table::KeyTable; use matrix::Matrix; pub use observer::{NullObserver, Observer}; pub use outcome::Outcome; @@ -226,9 +223,8 @@ const HALF_LINE_WINDOW: f64 = 10.0; const NARROW_WINDOW_RATIO: f64 = 2.0e4; const ASYMPTOTIC_MILLS_ALPHA: f64 = 100.0; -pub const N01: Gaussian = Gaussian::from_ms(0.0, 1.0); -pub const N00: Gaussian = Gaussian::from_ms(0.0, 0.0); -pub const N_INF: Gaussian = Gaussian::from_ms(0.0, f64::INFINITY); +pub(crate) const N00: Gaussian = Gaussian::from_ms(0.0, 0.0); +pub(crate) 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); diff --git a/src/storage/competitor_store.rs b/src/storage/competitor_store.rs index 6ac7789..c47314a 100644 --- a/src/storage/competitor_store.rs +++ b/src/storage/competitor_store.rs @@ -56,16 +56,16 @@ impl> CompetitorStore { self.get(idx).is_some() } + /// Test-only: no code path in the crate needs a count. + #[cfg(test)] #[must_use] pub fn len(&self) -> usize { self.n_present } - #[must_use] - pub fn is_empty(&self) -> bool { - self.n_present == 0 - } - + /// Test-only: iterating every competitor is an assertion helper, not part + /// of inference, which walks slices rather than the store. + #[cfg(test)] pub fn iter(&self) -> impl Iterator)> { self.competitors .iter() @@ -73,13 +73,6 @@ impl> CompetitorStore { .filter_map(|(i, slot)| slot.as_ref().map(|a| (Index(i), a))) } - pub fn iter_mut(&mut self) -> impl Iterator)> { - self.competitors - .iter_mut() - .enumerate() - .filter_map(|(i, slot)| slot.as_mut().map(|a| (Index(i), a))) - } - pub fn values_mut(&mut self) -> impl Iterator> { self.competitors.iter_mut().filter_map(|s| s.as_mut()) } diff --git a/src/time_slice.rs b/src/time_slice.rs index b00fd11..6518ac1 100644 --- a/src/time_slice.rs +++ b/src/time_slice.rs @@ -760,6 +760,8 @@ impl TimeSlice { } } + /// Test-only: reads the slice's shape back for assertions. + #[cfg(test)] pub fn get_composition(&self) -> Vec>> { self.events .iter() @@ -773,6 +775,8 @@ impl TimeSlice { .collect::>() } + /// Test-only: reads the slice's shape back for assertions. + #[cfg(test)] pub fn get_results(&self) -> Vec> { self.events .iter() @@ -887,7 +891,7 @@ mod tests { use super::*; use crate::{ - KeyTable, competitor::Competitor, drift::ConstantDrift, rating::Rating, + competitor::Competitor, drift::ConstantDrift, key_table::KeyTable, rating::Rating, storage::CompetitorStore, };