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

EventBuilder and record_winner/record_draw cannot set prior or drift_scale #37

New Issue
Closed
opened 2026-09-01 17:35:22 +00:00 by logaritmisk · 1 comment
logaritmisk commented 2026-09-01 17:35:22 +00:00
Owner
Copy Link
Copy Source

Three of the four ingestion routes cannot express competitor configuration. Only the typed add_events path can.

Noticed while documenting Member::with_drift_scale (#34) for the README (#35): the natural place to show the feature is the fluent builder, and the fluent builder cannot do it.

The gap

EventBuilder::team constructs members through Member::new and never exposes the rest of the builder (src/event_builder.rs:53-54):

pub fn team<I: IntoIterator<Item = K>>(mut self, keys: I) -> Self {
    let members: SmallVec<[Member<K>; 4]> = keys.into_iter().map(Member::new).collect();
    ...
}

weights is the only per-member setter it offers (src/event_builder.rs:70). There is no priors or drift_scales counterpart, so this cannot be written:

h.event(0)
    .team(["player"])
    .team(["layout_7"])            // want drift_scale 0.0 here
    .ranking([0, 1])
    .commit()?;

record_winner and record_draw are more absolute — both hand add_events_with_prior an empty map (src/history.rs:890, :914), so a competitor whose first appearance arrives through either one is permanently stuck on the history defaults.

Combined with first-appearance capture (#34), that is worse than an ergonomic wart: which route a competitor happens to arrive through decides whether they can ever be configured. See #38 for the lifecycle half of this.

Shape

Mirroring weights, which already records a length mismatch as MismatchedShape rather than panicking:

h.event(0)
    .team(["player", "layout_7"])
    .drift_scales([1.0, 0.0])
    .priors([None, Some(Gaussian::from_ms(0.0, 1.0))])
    .ranking([0, 1])
    .commit()?;

Worth considering instead: a members(...) escape hatch taking Member<K> values directly, which covers every present and future per-member field in one method rather than growing a parallel setter each time Member gains one. team stays the common case.

record_winner / record_draw are deliberately the two-argument convenience path; extending them is probably wrong. If #38 lands a pre-registration API, that covers them without touching their signatures — which is an argument for doing #38 first and keeping this issue to EventBuilder.

Tests

  • Each new setter reaches the same posterior as the equivalent typed add_events call.
  • A length mismatch returns MismatchedShape from commit, applies nothing, and cannot reach the history — matching the guarantee weights documents at src/event_builder.rs:60-65.
  • An invalid drift_scale surfaces as InvalidParameter from commit.
Three of the four ingestion routes cannot express competitor configuration. Only the typed `add_events` path can. Noticed while documenting `Member::with_drift_scale` (#34) for the README (#35): the natural place to show the feature is the fluent builder, and the fluent builder cannot do it. ## The gap `EventBuilder::team` constructs members through `Member::new` and never exposes the rest of the builder (`src/event_builder.rs:53-54`): ```rust pub fn team<I: IntoIterator<Item = K>>(mut self, keys: I) -> Self { let members: SmallVec<[Member<K>; 4]> = keys.into_iter().map(Member::new).collect(); ... } ``` `weights` is the only per-member setter it offers (`src/event_builder.rs:70`). There is no `priors` or `drift_scales` counterpart, so this cannot be written: ```rust h.event(0) .team(["player"]) .team(["layout_7"]) // want drift_scale 0.0 here .ranking([0, 1]) .commit()?; ``` `record_winner` and `record_draw` are more absolute — both hand `add_events_with_prior` an empty map (`src/history.rs:890`, `:914`), so a competitor whose first appearance arrives through either one is permanently stuck on the history defaults. Combined with first-appearance capture (#34), that is worse than an ergonomic wart: which route a competitor happens to arrive through decides whether they can ever be configured. See #38 for the lifecycle half of this. ## Shape Mirroring `weights`, which already records a length mismatch as `MismatchedShape` rather than panicking: ```rust h.event(0) .team(["player", "layout_7"]) .drift_scales([1.0, 0.0]) .priors([None, Some(Gaussian::from_ms(0.0, 1.0))]) .ranking([0, 1]) .commit()?; ``` Worth considering instead: a `members(...)` escape hatch taking `Member<K>` values directly, which covers every present and future per-member field in one method rather than growing a parallel setter each time `Member` gains one. `team` stays the common case. `record_winner` / `record_draw` are deliberately the two-argument convenience path; extending them is probably wrong. If #38 lands a pre-registration API, that covers them without touching their signatures — which is an argument for doing #38 first and keeping this issue to `EventBuilder`. ## Tests - Each new setter reaches the same posterior as the equivalent typed `add_events` call. - A length mismatch returns `MismatchedShape` from `commit`, applies nothing, and cannot reach the history — matching the guarantee `weights` documents at `src/event_builder.rs:60-65`. - An invalid `drift_scale` surfaces as `InvalidParameter` from `commit`.
logaritmisk added the apienhancement labels 2026-09-07 13:52:09 +00:00
logaritmisk commented 2026-09-08 13:54:51 +00:00
Author
Owner
Copy Link
Copy Source

Done for EventBuilder, in 7c6965c. record_winner/record_draw left alone, for the reason you gave and one you could not have known yet.

EventBuilder::members

Your "worth considering instead" won:

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()?;

team stays the common case. One method rather than priors and drift_scales beside weights, because 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, and it covers whatever Member gains next for free.

Your three tests, in tests/event_builder_members.rs: members reaches the typed path's posterior bit for bit; weights still records MismatchedShape on a members team and applies nothing; an invalid drift_scale surfaces as InvalidParameter from commit for -1.0, NaN and infinity, with nothing reaching the history.

There is a fourth, because the first three would all pass if members recorded the configuration and inference ignored it. A competitor pinned at drift_scale = 0.0 is checked not to move across slices, against an unpinned control in the same fit. Worth saying how that one nearly went wrong: I first asserted the pinned marginals were bit-identical across slices. They are not — each slice combines its own forward and backward messages, so the last bit moves. "Pinned" promises that no drift variance accumulates, not that the arithmetic is identical, and asserting the stronger thing would have pinned a property the model does not have.

record_winner / record_draw

You wrote that extending them is probably wrong, and that #38's pre-registration would cover them without touching their signatures. Both still hold — they are pub and positional, so adding parameters is breaking.

But the premise underneath has weakened. This issue says a competitor whose first appearance arrives through either one is "permanently stuck on the history defaults". That stopped being true at 8c087ad in 0.4.0, which made competitor configuration apply whenever it is supplied, including on a key the history already knows, with a forward refresh from the earliest affected slice. Measured on exactly the route this issue names:

first seen via record_winner, configured later : mu = 40.000000000
configured from the start                      : mu = 40.000000000
never configured                               : mu =  4.027250930

So it is repairable, and the gap is ergonomic rather than a trap. That removes the urgency argument for touching those two signatures. The remaining case for pre-registration is #38's own — declaring configuration before any event exists — which is a better-shaped feature than overloading the convenience path.

Closing this as the EventBuilder half, which is what your own reasoning scoped it to.

Done for `EventBuilder`, in `7c6965c`. `record_winner`/`record_draw` left alone, for the reason you gave and one you could not have known yet. ## `EventBuilder::members` Your "worth considering instead" won: ```rust 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()?; ``` `team` stays the common case. One method rather than `priors` and `drift_scales` beside `weights`, because 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, and it covers whatever `Member` gains next for free. Your three tests, in `tests/event_builder_members.rs`: `members` reaches the typed path's posterior bit for bit; `weights` still records `MismatchedShape` on a `members` team and applies nothing; an invalid `drift_scale` surfaces as `InvalidParameter` from `commit` for `-1.0`, NaN and infinity, with nothing reaching the history. There is a fourth, because the first three would all pass if `members` recorded the configuration and inference ignored it. A competitor pinned at `drift_scale = 0.0` is checked not to move across slices, against an unpinned control in the same fit. Worth saying how that one nearly went wrong: I first asserted the pinned marginals were bit-identical across slices. They are not — each slice combines its own forward and backward messages, so the last bit moves. "Pinned" promises that no drift variance accumulates, not that the arithmetic is identical, and asserting the stronger thing would have pinned a property the model does not have. ## `record_winner` / `record_draw` You wrote that extending them is probably wrong, and that #38's pre-registration would cover them without touching their signatures. Both still hold — they are `pub` and positional, so adding parameters is breaking. But the premise underneath has weakened. This issue says a competitor whose first appearance arrives through either one is **"permanently stuck on the history defaults"**. That stopped being true at `8c087ad` in 0.4.0, which made competitor configuration apply whenever it is supplied, including on a key the history already knows, with a forward refresh from the earliest affected slice. Measured on exactly the route this issue names: ``` first seen via record_winner, configured later : mu = 40.000000000 configured from the start : mu = 40.000000000 never configured : mu = 4.027250930 ``` So it is repairable, and the gap is ergonomic rather than a trap. That removes the urgency argument for touching those two signatures. The remaining case for pre-registration is #38's own — declaring configuration before any event exists — which is a better-shaped feature than overloading the convenience path. Closing this as the `EventBuilder` half, which is what your own reasoning scoped it to.
Sign in to join this conversation.
No Branch/Tag Specified
Branches Tags
Labels
Clear labels
api
Shape or ergonomics of the public surface
 blocked
Waiting on another issue or an external decision
 breaking
Resolving it changes the public API; wants a major bump
 bug
Behaviour is wrong, including silently wrong
 decision
Needs a call to be made, not code to be written
 docs
Documentation, including doc comments and the README
 enhancement
New capability the crate does not have
 infra
CI, release process, packaging
 numerics
Floating-point accuracy, cancellation, convergence maths
 performance
Speed or memory
 refactor
Internal cleanup with no intended behaviour change
 testing
Test coverage and test infrastructure
No labels api enhancement
Milestone
No items
No Milestone
Projects
Clear projects
No projects
Assignees
Clear assignees
logaritmisk
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: logaritmisk/trueskill-tt#37