From 911b48faba70f05df7af16d08cd77b92b5bb0063 Mon Sep 17 00:00:00 2001 From: Anders Olsson Date: Tue, 8 Sep 2026 12:32:00 +0200 Subject: [PATCH] feat: add EventBuilder::members for per-member configuration MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `EventBuilder` could set weights and nothing else, so `prior` and `drift_scale` were reachable only through the typed `Event`/`Team`/`Member` shape plus `add_events`. Which ingestion route a competitor arrived through decided whether it could be configured. `members(...)` takes `Member` values directly, so `Member`'s own builder expresses everything. `team(...)` stays the common case. One escape hatch rather than `priors` and `drift_scales` setters beside `weights`, as the issue suggested and then argued against itself: a parallel array per field means a parallel length check per field, and each one is a new way to get the lengths wrong. `Member` already has a builder; this just lets the fluent path reach it. `record_winner`/`record_draw` are deliberately left alone. They are the two-argument convenience path, and extending them would be a breaking signature change. The issue's reason for wanting them extended has also weakened: it said a competitor arriving through them was "permanently stuck on the history defaults", and since 8c087ad that is no longer true — a later `add_events` carrying the `Member` refits the whole history. Measured, late configuration through that route reaches mu 40.000000000, identical to configuring from the start. Refs #37 Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_011hcFjNDmHXZF8URGLku5zZ --- README.md | 7 +- src/event_builder.rs | 36 ++++++ tests/event_builder_members.rs | 193 +++++++++++++++++++++++++++++++++ 3 files changed, 233 insertions(+), 3 deletions(-) create mode 100644 tests/event_builder_members.rs diff --git a/README.md b/README.md index 6411954..fdf3c67 100644 --- a/README.md +++ b/README.md @@ -144,9 +144,10 @@ competitor within a single batch is `InferenceError::ConflictingCompetitorConfig since events in a batch have no order. The scale must be finite and non-negative; ingestion otherwise fails with `InferenceError::InvalidParameter`. -Note that the fluent `EventBuilder` (`h.event(t).team([...])`) sets weights but -not `drift_scale` or `prior`; those need the typed `Event` / `Team` / `Member` -shape shown above. +The fluent `EventBuilder` reaches this too: `.team([...])` is the common case +and leaves both unset, while `.members([...])` takes `Member` values directly, +so `h.event(t).members([Member::new("layout_7").with_drift_scale(0.0)])` is +equivalent to the typed shape above. ## Scored outcomes diff --git a/src/event_builder.rs b/src/event_builder.rs index 01fca05..ecc1555 100644 --- a/src/event_builder.rs +++ b/src/event_builder.rs @@ -50,6 +50,8 @@ where } /// Add a team by its member keys (weight 1.0 each, no prior overrides). + /// + /// Use [`EventBuilder::members`] to set `prior` or `drift_scale`. pub fn team>(mut self, keys: I) -> Self { let members: SmallVec<[Member; 4]> = keys.into_iter().map(Member::new).collect(); self.event.teams.push(Team { members }); @@ -57,6 +59,40 @@ where self } + /// Add a team from fully-specified [`Member`] values. + /// + /// [`EventBuilder::team`] is the common case and builds members with + /// `Member::new`, which leaves `prior` and `drift_scale` unset. This is the + /// escape hatch for when they matter: + /// + /// ``` + /// # use trueskill_tt::{Gaussian, History, Member}; + /// # let mut h = History::builder().build(); + /// h.event(0) + /// .team(["player"]) + /// .members([Member::new("layout_7") + /// .with_drift_scale(0.0) + /// .with_prior(Gaussian::from_ms(0.0, 1.0))]) + /// .ranking([0, 1]) + /// .commit()?; + /// # Ok::<(), trueskill_tt::InferenceError>(()) + /// ``` + /// + /// One method rather than a `priors` and a `drift_scales` setter beside + /// `weights`: those would have to grow a parallel array — and a parallel + /// length check — every time `Member` gains a field, and each one would be + /// a new way to get the lengths wrong. `Member`'s own builder already + /// expresses all of it. + /// + /// `prior` and `drift_scale` are competitor configuration rather than + /// per-event values; see [`Member`] for what that means for a key the + /// history already knows. + pub fn members>>(mut self, members: I) -> Self { + self.event.teams.push(Team::with_members(members)); + self.current_team_idx = Some(self.event.teams.len() - 1); + self + } + /// Set per-member weights for the most recently added team. /// /// A length mismatch is recorded and returned by [`EventBuilder::commit`] diff --git a/tests/event_builder_members.rs b/tests/event_builder_members.rs new file mode 100644 index 0000000..eb5ccc8 --- /dev/null +++ b/tests/event_builder_members.rs @@ -0,0 +1,193 @@ +//! `EventBuilder::members` must reach exactly what the typed path reaches. +//! +//! Before this existed, `EventBuilder` could set weights and nothing else, so +//! `prior` and `drift_scale` were expressible only through `Event`/`Team`/ +//! `Member` + `add_events`. Which ingestion route a competitor arrived through +//! decided whether it could be configured at all. + +use smallvec::smallvec; +use trueskill_tt::{ + ConstantDrift, ConvergenceOptions, Event, Gaussian, History, InferenceError, Member, Outcome, + Team, +}; + +type H = History; + +fn history() -> H { + History::builder() + .mu(0.0) + .sigma(6.0) + .beta(1.0) + .score_sigma(2.0) + .drift(ConstantDrift(0.5)) + .convergence(ConvergenceOptions { + max_iter: 20_000, + epsilon: 1e-13, + alpha: 1.0, + }) + .build() +} + +const PRIOR: Gaussian = Gaussian::from_ms(3.0, 1.5); + +/// The contract that makes the escape hatch worth having: same configuration, +/// same fit, bit for bit. +#[test] +fn members_matches_the_typed_path_exactly() { + let mut typed = history(); + typed + .add_events(vec![Event { + time: 1, + teams: smallvec![ + Team::with_members([Member::new("player")]), + Team::with_members([Member::new("layout_7") + .with_drift_scale(0.0) + .with_prior(PRIOR)]), + ], + outcome: Outcome::scores([5.0, 2.0]), + }]) + .unwrap(); + assert!(typed.converge().unwrap().converged); + + let mut fluent = history(); + fluent + .event(1) + .team(["player"]) + .members([Member::new("layout_7") + .with_drift_scale(0.0) + .with_prior(PRIOR)]) + .scores([5.0, 2.0]) + .commit() + .unwrap(); + assert!(fluent.converge().unwrap().converged); + + for key in ["player", "layout_7"] { + let a = typed.current_skill(&key).unwrap(); + let b = fluent.current_skill(&key).unwrap(); + assert_eq!(a.pi(), b.pi(), "{key} pi"); + assert_eq!(a.tau(), b.tau(), "{key} tau"); + } +} + +/// The configuration has to actually take effect, not merely round-trip: a +/// competitor pinned with `drift_scale = 0.0` must not move across slices, +/// where an unpinned one does. +/// +/// The comparison is against a control rather than against a fixed epsilon. +/// Pinned marginals are not bit-identical across slices — each slice combines +/// its own forward and backward messages, so the arithmetic order differs and +/// the last bit moves. What "pinned" promises is that no drift variance +/// accumulates, and the control is what makes that measurable. +#[test] +fn a_drift_scale_set_through_members_is_applied() { + fn spread(h: &H, key: &'static str) -> f64 { + let curve = h.learning_curve(&key); + assert!(curve.len() >= 2, "{key}: expected several appearances"); + let (lo, hi) = curve.iter().fold((f64::MAX, f64::MIN), |(lo, hi), (_, g)| { + (lo.min(g.sigma()), hi.max(g.sigma())) + }); + (hi - lo) / hi + } + + let mut h = history(); + for t in 1..=4 { + h.event(t) + .team(["player"]) + .members([Member::new("pinned").with_drift_scale(0.0)]) + .scores([5.0, 2.0]) + .commit() + .unwrap(); + // Same shape, no pinning: the control. + h.event(t) + .team(["rival"]) + .team(["drifting"]) + .scores([5.0, 2.0]) + .commit() + .unwrap(); + } + assert!(h.converge().unwrap().converged); + + let pinned = spread(&h, "pinned"); + let drifting = spread(&h, "drifting"); + assert!(pinned < 1e-9, "pinned competitor moved: {pinned:e}"); + assert!( + drifting > 1e-3, + "control did not move, so the test proves nothing: {drifting:e}" + ); +} + +/// `weights` still applies to a team added through `members`, and still +/// records a mismatch rather than partially applying it. +#[test] +fn weights_still_guards_a_members_team() { + let mut h = history(); + let err = h + .event(1) + .team(["a"]) + .members([Member::new("b"), Member::new("c")]) + .weights([1.0]) + .winner(0) + .commit() + .unwrap_err(); + assert!( + matches!( + err, + InferenceError::MismatchedShape { + kind: "weights", + expected: 2, + got: 1 + } + ), + "{err:?}" + ); + assert!(h.current_skill(&"b").is_none(), "nothing may reach history"); +} + +/// An invalid `drift_scale` surfaces from `commit`, not from a panic and not +/// silently. +#[test] +fn an_invalid_drift_scale_surfaces_from_commit() { + for bad in [-1.0, f64::NAN, f64::INFINITY] { + let mut h = history(); + let err = h + .event(1) + .team(["a"]) + .members([Member::new("b").with_drift_scale(bad)]) + .winner(0) + .commit() + .unwrap_err(); + assert!( + matches!( + err, + InferenceError::InvalidParameter { + name: "drift_scale", + .. + } + ), + "{bad}: {err:?}" + ); + assert!(h.current_skill(&"b").is_none(), "{bad} reached the history"); + } +} + +/// `members` and `team` compose in either order. +#[test] +fn members_and_team_interleave() { + let mut h = history(); + h.event(1) + .members([Member::new("a").with_prior(PRIOR)]) + .team(["b"]) + .scores([3.0, 1.0]) + .commit() + .unwrap(); + h.event(2) + .team(["b"]) + .members([Member::new("c").with_prior(PRIOR)]) + .scores([2.0, 4.0]) + .commit() + .unwrap(); + assert!(h.converge().unwrap().converged); + for key in ["a", "b", "c"] { + assert!(h.current_skill(&key).is_some(), "{key} missing"); + } +}