From dc1f4d5847a2d5c3309192fd6152f71423a1f592 Mon Sep 17 00:00:00 2001 From: Anders Olsson Date: Wed, 9 Sep 2026 20:19:16 +0200 Subject: [PATCH] fix!: make the Time generic reachable MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `History` has always been generic over the time axis, `Untimed` has always been exported, and `Drift` is generic specifically so that "seasonal or calendar-aware drift is expressible without going through i64". None of it was reachable from a downstream crate. Every construction route pinned `T = i64`: `History::builder()`, `History::builder_with_key()`, and the only `Default` impl on `HistoryBuilder`. Its fields are private and it had no `new`. So all three escape routes failed to compile, and a consumer with domain timestamps had to convert to i64 — which is the exact thing the parameter exists to avoid. One of `History`'s four type parameters was paid for at every signature and could never be varied. `Default` is now generic over `T` and `K`, `HistoryBuilder::new()` exists, and `time_type::()` / `key_type::()` join `drift` and `observer` as type-changing setters: History::builder().time_type::().build() History::builder().key_type::().build() HistoryBuilder::::new().build() `key_type` replaces `builder_with_key`, which could not be turbofished — `K` sat on the impl rather than the function, so callers had to spell `History::::builder_with_key()`. 18 call sites across 15 files migrated. tests/time_axis.rs is the part that matters. NOTHING in the repository constructed a non-i64 history, which is precisely why this survived, so the fix is only half done without a test that exercises the generic. It defines a `Season(u16)` time type and a `SeasonalDrift` that accumulates between seasons but not within one — the calendar-aware case the trait's docs cite — and checks the whole path: fit, converge, and read a learning curve whose times come back as `Season`, not as integers. Two of the six tests are controls rather than assertions about output. `Untimed` must ignore drift entirely, since elapsed is always zero, so gamma 0.0 and gamma 5.0 must agree bit for bit. And a custom `Drift` must actually widen a gap across seasons, or the test above would pass whether or not the drift was consulted at all. The README's ticked "Generalise a time axis" box is now true. BREAKING CHANGE: `History::builder_with_key()` is removed. Use `History::builder().key_type::()`. Closes #68 Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_011hcFjNDmHXZF8URGLku5zZ --- benches/history_converge.rs | 3 +- benches/ingest.rs | 6 +- benches/joint.rs | 3 +- benches/scored.rs | 3 +- examples/atp.rs | 3 +- src/history.rs | 112 ++++++++++++++--- tests/convergence_strictness.rs | 3 +- tests/cross_process_determinism.rs | 3 +- tests/degenerate_inputs.rs | 6 +- tests/determinism.rs | 3 +- tests/ingestion_equivalence.rs | 6 +- tests/joint_handle.rs | 3 +- tests/large_history_converges_finite.rs | 3 +- tests/marginal_calibration.rs | 3 +- tests/reconvergence_equivalence.rs | 12 +- tests/time_axis.rs | 152 ++++++++++++++++++++++++ 16 files changed, 286 insertions(+), 38 deletions(-) create mode 100644 tests/time_axis.rs diff --git a/benches/history_converge.rs b/benches/history_converge.rs index f623c37..95cd5f1 100644 --- a/benches/history_converge.rs +++ b/benches/history_converge.rs @@ -43,7 +43,8 @@ fn build_history_1v1( rng }; - let mut h = History::::builder_with_key() + let mut h = History::builder() + .key_type::() .mu(25.0) .sigma(25.0 / 3.0) .beta(25.0 / 6.0) diff --git a/benches/ingest.rs b/benches/ingest.rs index 0bac7a3..16cb5b0 100644 --- a/benches/ingest.rs +++ b/benches/ingest.rs @@ -32,7 +32,8 @@ fn bench_ingest(c: &mut Criterion) { b.iter_batched( || events(n, 0), |evs| { - let mut h: History = History::builder_with_key().build(); + let mut h: History = + History::builder().key_type::().build(); for ev in evs { h.add_events(std::iter::once(ev)).unwrap(); } @@ -46,7 +47,8 @@ fn bench_ingest(c: &mut Criterion) { b.iter_batched( || events(n, 0), |evs| { - let mut h: History = History::builder_with_key().build(); + let mut h: History = + History::builder().key_type::().build(); h.add_events(evs).unwrap(); black_box(h.time_slices_len()) }, diff --git a/benches/joint.rs b/benches/joint.rs index e611979..41f358b 100644 --- a/benches/joint.rs +++ b/benches/joint.rs @@ -11,7 +11,8 @@ use trueskill_tt::{ConstantDrift, ConvergenceOptions, Event, History, Member, Ou /// 30 slices of 8 duels: 480 appearances over 100 competitors. fn fitted() -> History { - let mut h: History = History::builder_with_key() + let mut h: History = History::builder() + .key_type::() .mu(0.0) .sigma(6.0) .beta(1.0) diff --git a/benches/scored.rs b/benches/scored.rs index 1ee167f..f0f4b6a 100644 --- a/benches/scored.rs +++ b/benches/scored.rs @@ -5,7 +5,8 @@ use trueskill_tt::{ConstantDrift, Event, History, Member, Outcome, Team}; fn bench_scored_history(c: &mut Criterion) { c.bench_function("scored_history_60_events_30_iter", |bencher| { bencher.iter(|| { - let mut h: History = History::builder_with_key() + let mut h: History = History::builder() + .key_type::() .mu(25.0) .sigma(25.0 / 3.0) .beta(25.0 / 6.0) diff --git a/examples/atp.rs b/examples/atp.rs index e7bd5a8..df9cb93 100644 --- a/examples/atp.rs +++ b/examples/atp.rs @@ -42,7 +42,8 @@ fn main() { } } - let mut hist: History = History::builder_with_key() + let mut hist: History = History::builder() + .key_type::() .sigma(1.6) .drift(ConstantDrift::new(0.036)) .convergence(trueskill_tt::ConvergenceOptions { diff --git a/src/history.rs b/src/history.rs index bfcae49..b2ae468 100644 --- a/src/history.rs +++ b/src/history.rs @@ -182,6 +182,73 @@ impl, O: Observer, K: Eq + Hash + Clone> HistoryBuilder< self } + /// Change the time axis, keeping every other setting. + /// + /// The same shape as [`HistoryBuilder::drift`] and + /// [`HistoryBuilder::observer`], which already move between type + /// parameters. Call it before setting a drift that is specific to one time + /// type, since the stored drift and observer must also be valid for `T2`. + /// + /// ``` + /// # use trueskill_tt::{History, Untimed}; + /// let mut h = History::builder().time_type::().build(); + /// h.record_winner(&"alice", &"bob", Untimed)?; + /// h.converge()?; + /// assert!(h.current_skill(&"alice").unwrap().mu() > 0.0); + /// # Ok::<(), trueskill_tt::InferenceError>(()) + /// ``` + #[must_use] + pub fn time_type(self) -> HistoryBuilder + where + T2: Time, + D: Drift, + O: Observer, + { + HistoryBuilder { + mu: self.mu, + sigma: self.sigma, + beta: self.beta, + drift: self.drift, + p_draw: self.p_draw, + score_sigma: self.score_sigma, + convergence: self.convergence, + observer: self.observer, + unknown_keys: self.unknown_keys, + _time: PhantomData, + _key: PhantomData, + } + } + + /// Change the key type, keeping every other setting. + /// + /// Replaces the former `History::builder_with_key`, which could not be + /// turbofished — `K` sat on the `impl` rather than the function, so + /// `History::builder_with_key::()` was a compile error and callers + /// had to spell the whole `History::builder().key_type::()`. + /// + /// ``` + /// # use trueskill_tt::History; + /// let mut h = History::builder().key_type::().build(); + /// h.record_winner(&"alice".to_string(), &"bob".to_string(), 1)?; + /// # Ok::<(), trueskill_tt::InferenceError>(()) + /// ``` + #[must_use] + pub fn key_type(self) -> HistoryBuilder { + HistoryBuilder { + mu: self.mu, + sigma: self.sigma, + beta: self.beta, + drift: self.drift, + p_draw: self.p_draw, + score_sigma: self.score_sigma, + convergence: self.convergence, + observer: self.observer, + unknown_keys: self.unknown_keys, + _time: PhantomData, + _key: PhantomData, + } + } + pub fn observer>(self, observer: O2) -> HistoryBuilder { HistoryBuilder { mu: self.mu, @@ -218,7 +285,14 @@ impl, O: Observer, K: Eq + Hash + Clone> HistoryBuilder< } } -impl Default for HistoryBuilder { +/// Generic over the time axis and the key type, so a builder exists for every +/// `T: Time` rather than only for `i64`. +/// +/// It used to be implemented for the `i64`/`&'static str` instantiation alone. +/// That, plus private fields and no `new`, meant a downstream crate could not +/// construct a `History` on any other time axis at all — `Untimed` and every +/// custom `Drift` were public but unreachable. +impl Default for HistoryBuilder { fn default() -> Self { Self { mu: MU, @@ -350,23 +424,25 @@ impl History { } } -impl History { - /// Like `builder()` but uses a custom key type `K` instead of the default `&'static str`. +impl HistoryBuilder { + /// A builder on any time axis and key type, with the default drift and no + /// observer. + /// + /// [`History::builder`] is the common case and pins `T = i64`, + /// `K = &'static str`. Reach for this — or for the type-changing + /// [`HistoryBuilder::time_type`] / [`HistoryBuilder::key_type`] — when + /// either needs to be something else. + /// + /// ``` + /// # use trueskill_tt::{History, HistoryBuilder, Untimed}; + /// let mut h = HistoryBuilder::::new().build(); + /// h.record_winner(&"alice".to_string(), &"bob".to_string(), Untimed)?; + /// h.converge()?; + /// # Ok::<(), trueskill_tt::InferenceError>(()) + /// ``` #[must_use] - pub fn builder_with_key() -> HistoryBuilder { - HistoryBuilder { - mu: MU, - sigma: SIGMA, - beta: BETA, - drift: ConstantDrift::new(GAMMA), - p_draw: P_DRAW, - score_sigma: 1.0, - convergence: ConvergenceOptions::default(), - observer: NullObserver, - unknown_keys: crate::UnknownKeys::default(), - _time: PhantomData, - _key: PhantomData, - } + pub fn new() -> Self { + Self::default() } } @@ -2545,7 +2621,7 @@ mod tests { fn per_slice_footprint_is_independent_of_index_magnitude() { fn total_skill_slots(high_indices: bool) -> usize { let mut h: History = - History::builder_with_key().build(); + History::builder().key_type::().build(); for i in 0..2_000 { h.intern(&format!("k{i:05}")); diff --git a/tests/convergence_strictness.rs b/tests/convergence_strictness.rs index fa18893..03ab150 100644 --- a/tests/convergence_strictness.rs +++ b/tests/convergence_strictness.rs @@ -107,7 +107,8 @@ fn the_two_agree_on_a_converged_fit() { /// 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() + let mut h: History = History::builder() + .key_type::() .mu(0.0) .sigma(6.0) .beta(1.0) diff --git a/tests/cross_process_determinism.rs b/tests/cross_process_determinism.rs index 79cf320..0157cf8 100644 --- a/tests/cross_process_determinism.rs +++ b/tests/cross_process_determinism.rs @@ -27,7 +27,8 @@ const RUNS: usize = 40; type H = History; fn fitted() -> H { - let mut h: H = History::builder_with_key() + let mut h: H = History::builder() + .key_type::() .mu(0.0) .sigma(6.0) .beta(1.0) diff --git a/tests/degenerate_inputs.rs b/tests/degenerate_inputs.rs index 61f3c94..e3e4ef7 100644 --- a/tests/degenerate_inputs.rs +++ b/tests/degenerate_inputs.rs @@ -126,8 +126,10 @@ fn empty_history_converges_trivially() { /// indexed out of bounds in release, so this must run in both profiles. #[test] fn converge_on_an_empty_history_with_owned_keys() { - let mut history: History = - History::builder_with_key().score_sigma(5.0).build(); + let mut history: History = History::builder() + .key_type::() + .score_sigma(5.0) + .build(); let report = history.converge().unwrap(); diff --git a/tests/determinism.rs b/tests/determinism.rs index bd6657c..bd14877 100644 --- a/tests/determinism.rs +++ b/tests/determinism.rs @@ -39,7 +39,8 @@ struct Fingerprint { } fn build_and_converge() -> Fingerprint { - let mut h = History::::builder_with_key() + let mut h = History::builder() + .key_type::() .mu(25.0) .sigma(25.0 / 3.0) .beta(25.0 / 6.0) diff --git a/tests/ingestion_equivalence.rs b/tests/ingestion_equivalence.rs index c50f708..88c2ba3 100644 --- a/tests/ingestion_equivalence.rs +++ b/tests/ingestion_equivalence.rs @@ -47,8 +47,10 @@ fn configured_event(a: &str, b: &str, time: i64, scale: f64) -> Event>, batched: bool) -> Vec<(String, Gaussian)> { - let mut h: History = - History::builder_with_key().convergence(tight()).build(); + let mut h: History = History::builder() + .key_type::() + .convergence(tight()) + .build(); if batched { h.add_events(events).unwrap(); diff --git a/tests/joint_handle.rs b/tests/joint_handle.rs index 5dc5a99..7aa2680 100644 --- a/tests/joint_handle.rs +++ b/tests/joint_handle.rs @@ -279,7 +279,8 @@ fn unseen_competitors_match_the_one_shot_path() { #[test] fn a_drift_too_small_to_represent_collapses_rather_than_corrupting() { fn variance(scale: f64) -> f64 { - let mut h: History = History::builder_with_key() + let mut h: History = History::builder() + .key_type::() .mu(0.0) .sigma(6.0) .beta(1.0) diff --git a/tests/large_history_converges_finite.rs b/tests/large_history_converges_finite.rs index 50b376b..48d4756 100644 --- a/tests/large_history_converges_finite.rs +++ b/tests/large_history_converges_finite.rs @@ -24,7 +24,8 @@ impl Lcg { } fn nan_after_fit(players: usize) -> usize { - let mut h: History = History::builder_with_key() + let mut h: History = History::builder() + .key_type::() .beta(1.0) .sigma(6.0) .drift(ConstantDrift::new(0.1)) diff --git a/tests/marginal_calibration.rs b/tests/marginal_calibration.rs index bfe8672..2eccf55 100644 --- a/tests/marginal_calibration.rs +++ b/tests/marginal_calibration.rs @@ -322,7 +322,8 @@ fn cost_scaling() { use std::time::Instant; for n in [50usize, 100, 200, 400, 800] { let names: Vec = (0..n).map(|i| format!("c{i}")).collect(); - let mut h: History = History::builder_with_key() + let mut h: History = History::builder() + .key_type::() .score_sigma(2.0) .drift(ConstantDrift::new(0.0)) .convergence(ConvergenceOptions { diff --git a/tests/reconvergence_equivalence.rs b/tests/reconvergence_equivalence.rs index ea08d35..aa28a6a 100644 --- a/tests/reconvergence_equivalence.rs +++ b/tests/reconvergence_equivalence.rs @@ -35,8 +35,10 @@ fn ev(a: &str, b: &str, time: i64) -> Event { /// Ingest each chunk in turn, converging fully after every one. fn fit_in_chunks(chunks: Vec) -> Vec<(String, Gaussian)> { - let mut h: History = - History::builder_with_key().convergence(tight()).build(); + let mut h: History = History::builder() + .key_type::() + .convergence(tight()) + .build(); for chunk in chunks { h.add_events(chunk).unwrap(); @@ -150,8 +152,10 @@ fn re_converging_an_unchanged_history_costs_one_iteration() { let (early, late) = fixture(); let all: Vec<_> = early.into_iter().chain(late).collect(); - let mut h: History = - History::builder_with_key().convergence(tight()).build(); + let mut h: History = History::builder() + .key_type::() + .convergence(tight()) + .build(); h.add_events(all).unwrap(); let first = h.converge().unwrap(); assert!(first.converged); diff --git a/tests/time_axis.rs b/tests/time_axis.rs new file mode 100644 index 0000000..7c38438 --- /dev/null +++ b/tests/time_axis.rs @@ -0,0 +1,152 @@ +//! The `Time` generic, exercised end to end. +//! +//! `History` has always been generic over the time axis, `Untimed` +//! has always been exported, and `Drift` is generic specifically so that +//! "seasonal or calendar-aware drift is expressible without going through +//! `i64`". None of it was reachable: every construction route pinned `T = i64`, +//! `HistoryBuilder`'s fields are private, and its `Default` existed only for the +//! `i64` instantiation. +//! +//! Nothing in the repository constructed a non-`i64` history, which is why that +//! went unnoticed. This file is the guard against it recurring — it is as much +//! about the generic being *exercised* as about any single assertion. + +use trueskill_tt::{ConstantDrift, Drift, History, HistoryBuilder, Time, Untimed}; + +/// A domain time type: a season number. Exactly what the `Time` trait exists +/// to support, and what a consumer with `chrono` dates would write. +#[derive(Copy, Clone, Debug, PartialEq, Eq, PartialOrd, Ord)] +struct Season(u16); + +impl Time for Season { + fn elapsed_to(&self, later: &Self) -> i64 { + i64::from(later.0.saturating_sub(self.0)) + } +} + +/// Drift that only accumulates between seasons, not within one — the +/// calendar-aware case the trait's own docs cite. +#[derive(Copy, Clone, Debug)] +struct SeasonalDrift { + per_season: f64, +} + +impl Drift for SeasonalDrift { + fn variance_delta(&self, from: &Season, to: &Season) -> f64 { + self.variance_for_elapsed(from.elapsed_to(to)) + } + + fn variance_for_elapsed(&self, elapsed: i64) -> f64 { + elapsed.max(0) as f64 * self.per_season * self.per_season + } +} + +#[test] +fn an_untimed_history_fits_through_the_builder() { + let mut h = History::builder().time_type::().build(); + for _ in 0..5 { + h.record_winner(&"alice", &"bob", Untimed).unwrap(); + } + assert!(h.converge().unwrap().converged); + + let alice = h.current_skill(&"alice").unwrap(); + let bob = h.current_skill(&"bob").unwrap(); + assert!(alice.mu() > bob.mu(), "{alice:?} vs {bob:?}"); + assert!(alice.sigma().is_finite() && alice.sigma() > 0.0); +} + +/// `Untimed::elapsed_to` is always 0, so no drift accumulates however many +/// events there are. That is the property the type exists for, and it had never +/// been checked. +#[test] +fn untimed_accumulates_no_drift() { + fn final_sigma(time: T, drift: ConstantDrift) -> f64 { + let mut h = History::builder().time_type::().drift(drift).build(); + for _ in 0..8 { + h.record_winner(&"a", &"b", time).unwrap(); + } + let _ = h.converge().unwrap(); + h.current_skill(&"a").unwrap().sigma() + } + + // Under Untimed the drift setting cannot matter, because elapsed is always 0. + let none = final_sigma(Untimed, ConstantDrift::new(0.0)); + let large = final_sigma(Untimed, ConstantDrift::new(5.0)); + assert_eq!( + none.to_bits(), + large.to_bits(), + "Untimed must ignore drift entirely: {none} vs {large}" + ); +} + +#[test] +fn a_custom_time_type_and_a_custom_drift_work_together() { + let mut h = History::builder() + .time_type::() + .drift(SeasonalDrift { per_season: 0.5 }) + .build(); + + for season in 1..=4u16 { + for _ in 0..3 { + h.record_winner(&"veteran", &"rookie", Season(season)) + .unwrap(); + } + } + assert!(h.converge().unwrap().converged); + + let curve = h.learning_curve(&"veteran"); + assert_eq!(curve.len(), 4, "one point per season: {curve:?}"); + for (season, g) in &curve { + assert!( + g.mu().is_finite() && g.sigma() > 0.0, + "season {season:?}: {g:?}" + ); + } + // Times come back as the domain type, not as an integer. + assert_eq!(curve[0].0, Season(1)); + assert_eq!(curve[3].0, Season(4)); +} + +/// Seasonal drift must actually widen a gap across seasons — otherwise the +/// custom `Drift` is being ignored and the test above would pass regardless. +#[test] +fn a_custom_drift_is_actually_consulted() { + fn sigma_with(per_season: f64) -> f64 { + let mut h = History::builder() + .time_type::() + .drift(SeasonalDrift { per_season }) + .build(); + for season in 1..=6u16 { + h.record_winner(&"a", &"b", Season(season)).unwrap(); + } + let _ = h.converge().unwrap(); + h.current_skill(&"a").unwrap().sigma() + } + + let still = sigma_with(0.0); + let drifting = sigma_with(2.0); + assert!( + drifting > still * 1.05, + "a drifting fit must be less certain: {drifting} vs {still}" + ); +} + +/// The other axis: a custom key type, through the same mechanism. +#[test] +fn key_type_replaces_builder_with_key() { + let mut h = History::builder().key_type::().build(); + h.record_winner(&"alice".to_string(), &"bob".to_string(), 1) + .unwrap(); + assert!(h.converge().unwrap().converged); + assert!(h.current_skill("alice").is_some()); +} + +/// Both axes at once, via the explicit constructor rather than the setters. +#[test] +fn new_constructs_on_any_axis_directly() { + let mut h = HistoryBuilder::::new().build(); + h.record_winner(&"a".to_string(), &"b".to_string(), Season(7)) + .unwrap(); + assert!(h.converge().unwrap().converged); + assert_eq!(h.learning_curve("a")[0].0, Season(7)); +}