logaritmisk/trueskill-tt
1
0
Fork 0
Code Issues 1 Pull Requests Packages Projects Releases Activity

refactor!: K comes first in History, HistoryBuilder and Joint

Browse Source 
`K` is the one type parameter people change, and it was last. Naming a
history in a struct field meant writing all four to say one thing:

    struct Ladder { history: History<i64, ConstantDrift, NullObserver, String> }
    struct Analysis<'h> { joint: Joint<'h, i64, ConstantDrift, NullObserver, &'static str> }

Now:

    struct Ladder { history: History<String> }
    struct Analysis<'h> { joint: Joint<'h> }

`History<K, T, D, O>`, all four defaulted. Bounds may reference later
parameters, so `D: Drift<T> = ConstantDrift` is legal in third position.
`Joint` gains the same defaults, so `Joint<'h, String>` spells it.

72 call sites swapped, and the reorder makes most of them shorter: 18
now read `History<String>` and the `&'static str` ones read `History`.
The two turbofished builders shrink from
`HistoryBuilder::<Untimed, _, _, String>::new()` to
`HistoryBuilder::<String, Untimed>::new()`.

`Joint` keeps `O` structurally, defaulted rather than removed. #72 notes
it never touches the observer, which is true — but it borrows the whole
`&'h History<K, T, D, O>` and calls `History::resolve_terms`, so dropping
the parameter means either a view type or moving that method off
`History`. The default already buys the entire user-visible benefit,
which was the spelling.

Refs #72.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011hcFjNDmHXZF8URGLku5zZ
This commit is contained in:
logaritmisk
2026-09-10 06:49:22 +02:00
co-authored by Claude Opus 5
parent d2ab4446ef
commit b553c630f5
29 changed files with 129 additions and 125 deletions
Download Patch File Download Diff File Expand all files Collapse all files
src/event_builder.rs
+2 -2
View File
@@ -45,7 +45,7 @@ where
O: Observer<T>,
K: Eq + std::hash::Hash + Clone,
{
history: &'h mut History<T, D, O, K>,
history: &'h mut History<K, T, D, O>,
event: Event<T, K>,
current_team_idx: Option<usize>,
/// First validation failure seen while building, surfaced by `commit`.
@@ -65,7 +65,7 @@ where
O: Observer<T>,
K: Eq + std::hash::Hash + Clone,
{
pub(crate) fn new(history: &'h mut History<T, D, O, K>, time: T) -> Self {
pub(crate) fn new(history: &'h mut History<K, T, D, O>, time: T) -> Self {
Self {
history,
event: Event {
src/history.rs
+80 -60
View File
@@ -32,6 +32,9 @@ use crate::{
/// an unknown key. None of them can be changed after `build`, because they
/// define the model the fit is of.
///
/// Parameterised as [`History`] is, `HistoryBuilder<K, T, D, O>`, with the
/// same defaults.
///
/// Two of the setters change the builder's *type* rather than a field —
/// [`HistoryBuilder::drift`] and [`HistoryBuilder::observer`] — so bind the
/// result rather than calling them on a `&mut`. [`HistoryBuilder::time_type`]
@@ -40,10 +43,10 @@ use crate::{
#[derive(Clone, Debug)]
#[must_use = "a builder does nothing until `.build()`"]
pub struct HistoryBuilder<
K: Eq + Hash + Clone = &'static str,
T: Time = i64,
D: Drift<T> = ConstantDrift,
O: Observer<T> = NullObserver,
K: Eq + Hash + Clone = &'static str,
> {
mu: f64,
sigma: f64,
@@ -58,7 +61,7 @@ pub struct HistoryBuilder<
_key: PhantomData<K>,
}
impl<T: Time, D: Drift<T>, O: Observer<T>, K: Eq + Hash + Clone> HistoryBuilder<T, D, O, K> {
impl<T: Time, D: Drift<T>, O: Observer<T>, K: Eq + Hash + Clone> HistoryBuilder<K, T, D, O> {
/// Prior mean skill.
///
/// # Panics
@@ -150,7 +153,7 @@ impl<T: Time, D: Drift<T>, O: Observer<T>, K: Eq + Hash + Clone> HistoryBuilder<
/// implementation. `converge` checks the variance each competitor actually
/// accumulates and reports `InvalidParameter` if it is negative or
/// non-finite.
pub fn drift<D2: Drift<T>>(self, drift: D2) -> HistoryBuilder<T, D2, O, K> {
pub fn drift<D2: Drift<T>>(self, drift: D2) -> HistoryBuilder<K, T, D2, O> {
HistoryBuilder {
drift,
mu: self.mu,
@@ -249,7 +252,7 @@ impl<T: Time, D: Drift<T>, O: Observer<T>, K: Eq + Hash + Clone> HistoryBuilder<
/// assert!(h.current_skill(&"alice").unwrap().mu() > 0.0);
/// # Ok::<(), trueskill_tt::InferenceError>(())
/// ```
pub fn time_type<T2>(self) -> HistoryBuilder<T2, D, O, K>
pub fn time_type<T2>(self) -> HistoryBuilder<K, T2, D, O>
where
T2: Time,
D: Drift<T2>,
@@ -283,7 +286,7 @@ impl<T: Time, D: Drift<T>, O: Observer<T>, K: Eq + Hash + Clone> HistoryBuilder<
/// h.record_winner(&"alice".to_string(), &"bob".to_string(), 1)?;
/// # Ok::<(), trueskill_tt::InferenceError>(())
/// ```
pub fn key_type<K2: Eq + Hash + Clone>(self) -> HistoryBuilder<T, D, O, K2> {
pub fn key_type<K2: Eq + Hash + Clone>(self) -> HistoryBuilder<K2, T, D, O> {
HistoryBuilder {
mu: self.mu,
sigma: self.sigma,
@@ -305,7 +308,7 @@ impl<T: Time, D: Drift<T>, O: Observer<T>, K: Eq + Hash + Clone> HistoryBuilder<
/// observer by value; to keep a handle on one that accumulates state, pass
/// an `Arc` and keep a clone, or read it back with
/// [`History::observer`] / [`History::into_observer`].
pub fn observer<O2: Observer<T>>(self, observer: O2) -> HistoryBuilder<T, D, O2, K> {
pub fn observer<O2: Observer<T>>(self, observer: O2) -> HistoryBuilder<K, T, D, O2> {
HistoryBuilder {
mu: self.mu,
sigma: self.sigma,
@@ -324,7 +327,7 @@ impl<T: Time, D: Drift<T>, O: Observer<T>, K: Eq + Hash + Clone> HistoryBuilder<
/// Finish configuring and produce an empty [`History`].
///
/// Every parameter was validated as it was set, so this cannot fail.
pub fn build(self) -> History<T, D, O, K> {
pub fn build(self) -> History<K, T, D, O> {
History {
size: 0,
time_slices: Vec::new(),
@@ -351,7 +354,7 @@ impl<T: Time, D: Drift<T>, O: Observer<T>, K: Eq + Hash + Clone> HistoryBuilder<
/// 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<T>` were public but unreachable.
impl<T: Time, K: Eq + Hash + Clone> Default for HistoryBuilder<T, ConstantDrift, NullObserver, K> {
impl<T: Time, K: Eq + Hash + Clone> Default for HistoryBuilder<K, T, ConstantDrift, NullObserver> {
fn default() -> Self {
Self {
mu: MU,
@@ -443,11 +446,25 @@ impl CompetitorConfig {
///
/// `tests/reconvergence_equivalence.rs` pins the path-independence this rests
/// on.
/// The top-level container: ingests events, runs forward/backward message
/// passing, and answers queries about the fit.
///
/// # Type parameters
///
/// `History<K, T, D, O>` — key type, time type, drift model, observer — all
/// defaulted, so `History` alone means `&'static str` keys on an `i64` time
/// axis with [`ConstantDrift`] and no observer, and `History<String>` is the
/// whole spelling for owned keys.
///
/// `K` is first because it is the one people change. It used to be last, so
/// naming a history in a struct field meant writing all four:
/// `History<i64, ConstantDrift, NullObserver, String>` to say "keys are
/// `String`". See #72.
pub struct History<
K: Eq + Hash + Clone = &'static str,
T: Time = i64,
D: Drift<T> = ConstantDrift,
O: Observer<T> = NullObserver,
K: Eq + Hash + Clone = &'static str,
> {
size: usize,
pub(crate) time_slices: Vec<TimeSlice<T>>,
@@ -470,25 +487,25 @@ pub struct History<
declared: HashMap<Index, CompetitorConfig>,
}
impl Default for History<i64, ConstantDrift, NullObserver, &'static str> {
impl Default for History {
fn default() -> Self {
HistoryBuilder::default().build()
}
}
impl History<i64, ConstantDrift, NullObserver, &'static str> {
impl History {
/// Start configuring a history.
///
/// The defaults are `i64` time, [`ConstantDrift`], no observer and
/// `&'static str` keys. Any of the four can be changed — the two type
/// parameters that no argument would pin are named with
/// [`HistoryBuilder::time_type`] and [`HistoryBuilder::key_type`].
pub fn builder() -> HistoryBuilder<i64, ConstantDrift, NullObserver, &'static str> {
pub fn builder() -> HistoryBuilder {
HistoryBuilder::default()
}
}
impl<T: Time, K: Eq + Hash + Clone> HistoryBuilder<T, ConstantDrift, NullObserver, K> {
impl<T: Time, K: Eq + Hash + Clone> HistoryBuilder<K, T, ConstantDrift, NullObserver> {
/// A builder on any time axis and key type, with the default drift and no
/// observer.
///
@@ -499,7 +516,7 @@ impl<T: Time, K: Eq + Hash + Clone> HistoryBuilder<T, ConstantDrift, NullObserve
///
/// ```
/// # use trueskill_tt::{History, HistoryBuilder, Untimed};
/// let mut h = HistoryBuilder::<Untimed, _, _, String>::new().build();
/// let mut h = HistoryBuilder::<String, Untimed>::new().build();
/// h.record_winner(&"alice".to_string(), &"bob".to_string(), Untimed)?;
/// h.converge()?;
/// # Ok::<(), trueskill_tt::InferenceError>(())
@@ -535,7 +552,7 @@ impl<T: Time, K: Eq + Hash + Clone> HistoryBuilder<T, ConstantDrift, NullObserve
}
}
impl<T: Time, D: Drift<T>, O: Observer<T>, K: Eq + Hash + Clone> History<T, D, O, K> {
impl<T: Time, D: Drift<T>, O: Observer<T>, K: Eq + Hash + Clone> History<K, T, D, O> {
/// Promote a key to its [`Index`], creating the entry if it is new.
///
/// Crate-internal since #73: interning reserves a storage slot and nothing
@@ -550,7 +567,7 @@ impl<T: Time, D: Drift<T>, O: Observer<T>, K: Eq + Hash + Clone> History<T, D, O
}
}
impl<T: Time, D: Drift<T>, O: Observer<T>, K: Eq + Hash + Clone> History<T, D, O, K> {
impl<T: Time, D: Drift<T>, O: Observer<T>, K: Eq + Hash + Clone> History<K, T, D, O> {
fn iteration(&mut self) -> (f64, f64) {
let mut step = (0.0, 0.0);
@@ -1577,7 +1594,7 @@ impl<T: Time, D: Drift<T>, O: Observer<T>, K: Eq + Hash + Clone> History<T, D, O
///
/// `JointUnavailable` if the history is empty, contains ranked events, or
/// yields a precision matrix that is not positive-definite.
pub fn joint(&self) -> Result<Joint<'_, T, D, O, K>, InferenceError> {
pub fn joint(&self) -> Result<Joint<'_, K, T, D, O>, InferenceError> {
if self.time_slices.is_empty() {
return Err(InferenceError::JointUnavailable {
reason: "the history has no events",
@@ -2048,7 +2065,7 @@ impl<T: Time, D: Drift<T>, O: Observer<T>, K: Eq + Hash + Clone> History<T, D, O
}
}
impl<T: Time, D: Drift<T>, O: Observer<T>, K: Eq + Hash + Clone> History<T, D, O, K> {
impl<T: Time, D: Drift<T>, O: Observer<T>, K: Eq + Hash + Clone> History<K, T, D, O> {
pub(crate) fn add_events_with_prior(
&mut self,
mut composition: Vec<Vec<Vec<Index>>>,
@@ -2654,7 +2671,7 @@ impl<T: Time, D: Drift<T>, O: Observer<T>, K: Eq + Hash + Clone> History<T, D, O
/// It exists at all because without it a consumer cannot `#[derive(Debug)]` on
/// any struct holding a `History`, which is how both known consumers store it.
impl<T: Time, D: Drift<T>, O: Observer<T>, K: Eq + Hash + Clone> std::fmt::Debug
for History<T, D, O, K>
for History<K, T, D, O>
{
fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
f.debug_struct("History")
@@ -2712,8 +2729,14 @@ impl<T: Time, D: Drift<T>, O: Observer<T>, K: Eq + Hash + Clone> std::fmt::Debug
/// so a competitor seen in the first and last of a hundred slices contributes
/// two variables, not a hundred.
#[must_use]
pub struct Joint<'h, T: Time, D: Drift<T>, O: Observer<T>, K: Eq + Hash + Clone> {
history: &'h History<T, D, O, K>,
pub struct Joint<
'h,
K: Eq + Hash + Clone = &'static str,
T: Time = i64,
D: Drift<T> = ConstantDrift,
O: Observer<T> = NullObserver,
> {
history: &'h History<K, T, D, O>,
cholesky: crate::joint::Cholesky,
/// `(row, slice)` of each competitor's latest appearance.
latest: HashMap<Index, (usize, usize)>,
@@ -2726,7 +2749,7 @@ pub struct Joint<'h, T: Time, D: Drift<T>, O: Observer<T>, K: Eq + Hash + Clone>
/// Deliberately does not print the factorisation, which is `n^2` floats and
/// would make a `{:?}` of a large joint unreadable and slow.
impl<T: Time, D: Drift<T>, O: Observer<T>, K: Eq + Hash + Clone> std::fmt::Debug
for Joint<'_, T, D, O, K>
for Joint<'_, K, T, D, O>
{
fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
f.debug_struct("Joint")
@@ -2735,7 +2758,7 @@ impl<T: Time, D: Drift<T>, O: Observer<T>, K: Eq + Hash + Clone> std::fmt::Debug
}
}
impl<T: Time, D: Drift<T>, O: Observer<T>, K: Eq + Hash + Clone> Joint<'_, T, D, O, K> {
impl<K: Eq + Hash + Clone, T: Time, D: Drift<T>, O: Observer<T>> Joint<'_, K, T, D, O> {
/// Number of variables in the joint: the history's appearances, after
/// collapsing consecutive pairs a competitor does not drift between.
///
@@ -2933,8 +2956,7 @@ mod tests {
#[test]
fn per_slice_footprint_is_independent_of_index_magnitude() {
fn total_skill_slots(high_indices: bool) -> usize {
let mut h: History<i64, ConstantDrift, NullObserver, String> =
History::builder().key_type::<String>().build();
let mut h: History<String> = History::builder().key_type::<String>().build();
for i in 0..2_000 {
h.intern(&format!("k{i:05}"));
@@ -3240,7 +3262,7 @@ mod tests {
#[test]
fn test_teams() {
let mut h: History<i64, _, _, &'static str> = History::builder()
let mut h: History = History::builder()
.mu(0.0)
.sigma(6.0)
.beta(1.0)
@@ -3346,7 +3368,7 @@ mod tests {
#[test]
fn test_add_events() {
let mut h: History<i64, _, _, &'static str> = History::builder()
let mut h: History = History::builder()
.mu(0.0)
.sigma(2.0)
.beta(1.0)
@@ -3443,7 +3465,7 @@ mod tests {
#[test]
fn test_only_add_events() {
let mut h: History<i64, _, _, &'static str> = History::builder()
let mut h: History = History::builder()
.mu(0.0)
.sigma(2.0)
.beta(1.0)
@@ -3542,7 +3564,7 @@ mod tests {
fn test_log_evidence() {
use crate::ConvergenceOptions;
let mut h: History<i64, _, _, &'static str> = History::builder().build();
let mut h: History = History::builder().build();
// empty results in the old API = team 0 wins; reproduce with Outcome::winner(0,2)
let events = make_events_1v1(
@@ -3598,7 +3620,7 @@ mod tests {
epsilon = 1e-4
);
let mut h2: History<i64, _, _, &'static str> = History::builder().build();
let mut h2: History = History::builder().build();
let events = make_events_1v1(
&[("a", "b"), ("b", "a")],
@@ -3616,7 +3638,7 @@ mod tests {
#[test]
fn test_add_events_with_time() {
let mut h: History<i64, _, _, &'static str> = History::builder()
let mut h: History = History::builder()
.mu(0.0)
.sigma(2.0)
.beta(1.0)
@@ -3719,7 +3741,7 @@ mod tests {
// second scenario: team-0 wins (empty results in old API), different composition order
let mut h2: History<i64, _, _, &'static str> = History::builder()
let mut h2: History = History::builder()
.mu(0.0)
.sigma(2.0)
.beta(1.0)
@@ -3823,7 +3845,7 @@ mod tests {
#[test]
fn test_1vs1_weighted() {
let mut h: History<i64, _, _, &'static str> = History::builder()
let mut h: History = History::builder()
.mu(2.0)
.sigma(6.0)
.beta(1.0)
@@ -3890,7 +3912,7 @@ mod tests {
fn test_converge_returns_report() {
use crate::ConvergenceOptions;
let mut h: History<i64, _, _, &'static str> = History::builder()
let mut h: History = History::builder()
.mu(0.0)
.sigma(2.0)
.beta(1.0)
@@ -3930,19 +3952,18 @@ mod tests {
fn history_propagates_convergence_to_inner_run_chain() {
use crate::ConvergenceOptions;
let events_for =
|h: &mut History<i64, ConstantDrift, crate::observer::NullObserver, &'static str>| {
h.event(0)
.team(["a"])
.team(["b"])
.team(["c"])
.team(["d"])
.ranking([0u32, 1, 2, 3])
.commit()
.unwrap();
};
let events_for = |h: &mut History| {
h.event(0)
.team(["a"])
.team(["b"])
.team(["c"])
.team(["d"])
.ranking([0u32, 1, 2, 3])
.commit()
.unwrap();
};
let mut h_capped: History<i64, _, _, &'static str> = History::builder()
let mut h_capped: History = History::builder()
.convergence(ConvergenceOptions {
max_iter: 1,
..ConvergenceOptions::default()
@@ -3953,7 +3974,7 @@ mod tests {
// result rather than an error.
let _ = h_capped.converge_partial().unwrap();
let mut h_full: History<i64, _, _, &'static str> = History::builder().build();
let mut h_full: History = History::builder().build();
events_for(&mut h_full);
let _ = h_full.converge().unwrap();
@@ -3978,23 +3999,22 @@ mod tests {
fn history_with_damping_reaches_same_fixed_point_as_undamped() {
use crate::ConvergenceOptions;
let events_for =
|h: &mut History<i64, ConstantDrift, crate::observer::NullObserver, &'static str>| {
h.event(0)
.team(["a"])
.team(["b"])
.team(["c"])
.team(["d"])
.ranking([0u32, 1, 2, 3])
.commit()
.unwrap();
};
let events_for = |h: &mut History| {
h.event(0)
.team(["a"])
.team(["b"])
.team(["c"])
.team(["d"])
.ranking([0u32, 1, 2, 3])
.commit()
.unwrap();
};
let mut h_undamped: History<i64, _, _, &'static str> = History::builder().build();
let mut h_undamped: History = History::builder().build();
events_for(&mut h_undamped);
let _ = h_undamped.converge().unwrap();
let mut h_damped: History<i64, _, _, &'static str> = History::builder()
let mut h_damped: History = History::builder()
.convergence(ConvergenceOptions {
alpha: 0.5,
max_iter: 200,