feat!: add History::register and History::rating, and reject config conflicts across batches

Three things #38 asked for, on a premise that had half dissolved. The
issue argued from "captured at first appearance", "missing it is silent"
and "missing it is permanent"; 8c087ad made configuration apply whenever
supplied and refit the whole history, so two of those are already gone.
What survived is the literal title — no way to say it before the first
event — plus the absence of any way to check.

`register(Member)` states configuration before anything is observed. It
takes the same `Member` ingestion takes, so there is one vocabulary
rather than two, and it creates the competitor immediately, which is what
makes it observable. It reaches a competitor first seen through
`record_winner`, the route #37 deliberately did not extend.

`rating(&key)` reads back what was stored. Every other accessor reports
what inference inferred; this reports what it was told, which is what
makes a configuration mistake detectable from outside the crate at all.

Conflicting configuration is now an error across batches, not only within
one. The `priors` map is rebuilt per `add_events` call, so a second batch
silently overwrote what a first declared, last-write-wins. That cut
directly against the invariant tests/ingestion_equivalence.rs exists to
protect: the same contradictory events errored when batched and
succeeded, order-dependently, when fed one at a time. Detection lives on
a new `declared` map on `History`, because a `Rating` cannot say whether
a value was chosen or inherited from the defaults — which is exactly the
distinction the check needs. Checked before anything mutates, so a
rejected batch leaves the history untouched.

`register` rejects a non-default `weight` rather than ignoring it. Weight
is per-event and has no meaning on a registration, and silently dropping
a field the caller set is the defect this whole area keeps producing.

The declarative `default_rating_for` closure is not here. It is the
better answer for ustat's actual case — thousands of keys matching a
rule, rather than enumerated — but it adds a `Fn` parameter to `History`,
which the issue itself flags as in tension with the crate's posture. That
wants its own decision rather than riding along.

BREAKING CHANGE: two different values for one competitor's `prior` or
`drift_scale` supplied across separate `add_events` calls now return
`ConflictingCompetitorConfig` instead of silently taking the later one.

Refs #38

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011hcFjNDmHXZF8URGLku5zZ
This commit is contained in:
2026-09-08 16:11:09 +02:00
co-authored by Claude Opus 5
parent 4f6360128d
commit e493f47e99
3 changed files with 514 additions and 1 deletions
+19
View File
@@ -118,6 +118,17 @@ pub enum InferenceError {
member: usize, member: usize,
key: String, key: String,
}, },
/// `History::register` was called for a competitor that already exists.
///
/// Registration states a competitor's configuration before anything has
/// been observed about them, so a competitor that already exists has
/// already been configured — by an earlier `register`, or by an event that
/// created them. Silently overwriting would reintroduce exactly the
/// order-dependence registration exists to remove.
///
/// To change an existing competitor's configuration, supply it on an event
/// through `Member`; that refits the whole history.
AlreadyRegistered { key: String },
/// A prediction was given a team with no members. /// A prediction was given a team with no members.
EmptyTeam { team: usize }, EmptyTeam { team: usize },
/// A joint posterior was requested where one cannot be formed exactly. /// A joint posterior was requested where one cannot be formed exactly.
@@ -197,6 +208,14 @@ impl fmt::Display for InferenceError {
with `lookup` or `current_skill` if that is not guaranteed)" with `lookup` or `current_skill` if that is not guaranteed)"
) )
} }
Self::AlreadyRegistered { key } => {
write!(
f,
"competitor {key} is already registered; registration states \
configuration before anything is observed, so re-registering \
would silently overwrite it"
)
}
Self::EmptyTeam { team } => { Self::EmptyTeam { team } => {
write!(f, "team {team} has no members") write!(f, "team {team} has no members")
} }
+157 -1
View File
@@ -6,6 +6,7 @@ use crate::{
convergence::{ConvergenceOptions, ConvergenceReport}, convergence::{ConvergenceOptions, ConvergenceReport},
drift::{ConstantDrift, Drift}, drift::{ConstantDrift, Drift},
error::InferenceError, error::InferenceError,
event::Member,
gaussian::Gaussian, gaussian::Gaussian,
key_table::KeyTable, key_table::KeyTable,
observer::{NullObserver, Observer}, observer::{NullObserver, Observer},
@@ -207,6 +208,7 @@ impl<T: Time, D: Drift<T>, O: Observer<T>, K: Eq + Hash + Clone> HistoryBuilder<
convergence: self.convergence, convergence: self.convergence,
observer: self.observer, observer: self.observer,
unknown_keys: self.unknown_keys, unknown_keys: self.unknown_keys,
declared: HashMap::new(),
} }
} }
} }
@@ -314,6 +316,12 @@ pub struct History<
convergence: ConvergenceOptions, convergence: ConvergenceOptions,
observer: O, observer: O,
unknown_keys: crate::UnknownKeys, unknown_keys: crate::UnknownKeys,
/// Competitor configuration explicitly declared so far, by whichever route.
///
/// Kept separate from the applied `Rating` because a `Rating` cannot say
/// whether a value was *chosen* or inherited from the history defaults,
/// and that is exactly the distinction a conflict check needs.
declared: HashMap<Index, CompetitorConfig>,
} }
impl Default for History<i64, ConstantDrift, NullObserver, &'static str> { impl Default for History<i64, ConstantDrift, NullObserver, &'static str> {
@@ -493,6 +501,111 @@ impl<T: Time, D: Drift<T>, O: Observer<T>, K: Eq + Hash + Clone> History<T, D, O
} }
/// Skill estimate at the latest time slice the competitor appears in. /// Skill estimate at the latest time slice the competitor appears in.
/// Configure a competitor before anything has been observed about them.
///
/// The configuration a competitor needs is often a property of the domain
/// rather than of any one event — "every layout is static", "this bot sits
/// at a known strength". Stating it per-event means every ingestion path
/// has to remember it, and the fluent and two-argument paths could not
/// state it at all.
///
/// ```
/// # use trueskill_tt::{History, Member};
/// let mut h = History::builder().build();
/// h.register(Member::new("layout_7").with_drift_scale(0.0))?;
///
/// // Reaches a competitor first seen through any route, including the
/// // two-argument one, which cannot carry configuration itself.
/// h.record_winner(&"player", &"layout_7", 1)?;
/// assert_eq!(h.rating(&"layout_7").unwrap().drift_scale(), 0.0);
/// # Ok::<(), trueskill_tt::InferenceError>(())
/// ```
///
/// The competitor exists from this point on, with no appearances, so
/// [`History::rating`] can read back what was actually stored — the
/// diagnostic that was previously missing entirely.
///
/// `weight` is per-event and has no meaning here, so a `Member` carrying a
/// non-default one is rejected rather than silently ignored.
///
/// # Errors
///
/// `AlreadyRegistered` if the competitor already exists, whether from an
/// earlier `register` or from an event. `InvalidParameter` for a `weight`
/// other than 1.0, or a `drift_scale` that is negative or non-finite.
pub fn register(&mut self, member: Member<K>) -> Result<(), InferenceError>
where
K: std::fmt::Debug,
{
if member.weight != 1.0 {
return Err(InferenceError::InvalidParameter {
name: "weight",
value: member.weight,
});
}
if let Some(scale) = member.drift_scale {
if !scale.is_finite() || scale < 0.0 {
return Err(InferenceError::InvalidParameter {
name: "drift_scale",
value: scale,
});
}
}
let key = format!("{:?}", member.key);
let idx = self.keys.get_or_create(&member.key);
if self.agents.contains(idx) {
return Err(InferenceError::AlreadyRegistered { key });
}
let mut rating = Rating::new(
Gaussian::from_ms(self.mu, self.sigma),
self.beta,
self.drift,
);
if let Some(prior) = member.prior {
rating.prior = prior;
}
if let Some(scale) = member.drift_scale {
rating.drift_scale = scale;
}
self.declared.insert(
idx,
CompetitorConfig {
prior: member.prior,
drift_scale: member.drift_scale,
},
);
self.agents.insert(
idx,
Competitor {
rating,
message: None,
last_time: None,
},
);
Ok(())
}
/// The configuration in force for a competitor, or `None` if the history
/// has never seen them.
///
/// Reads back what was actually stored, which is what makes a
/// configuration mistake detectable from outside the crate. Every other
/// accessor returns what inference *inferred*; this returns what it was
/// told.
#[must_use]
pub fn rating<Q>(&self, key: &Q) -> Option<Rating<T, D>>
where
K: std::borrow::Borrow<Q>,
Q: std::hash::Hash + Eq + ?Sized,
{
let idx = self.keys.get(key)?;
self.agents.contains(idx).then(|| self.agents[idx].rating)
}
pub fn current_skill<Q>(&self, key: &Q) -> Option<Gaussian> pub fn current_skill<Q>(&self, key: &Q) -> Option<Gaussian>
where where
K: std::borrow::Borrow<Q>, K: std::borrow::Borrow<Q>,
@@ -1647,6 +1760,47 @@ impl<T: Time, D: Drift<T>, O: Observer<T>, K: Eq + Hash + Clone> History<T, D, O
} }
} }
// Cross-batch conflict. The in-batch check upstream rejects one batch
// that sets a field twice; `priors` is rebuilt per call, so without
// this a *second* batch could quietly overwrite what a first one
// declared, last-write-wins.
//
// That asymmetry cut against the invariant `tests/ingestion_equivalence.rs`
// exists to protect: the same contradictory events errored when
// batched and succeeded, order-dependently, when fed one at a time.
// Checked before anything mutates, so a rejected batch leaves the
// history untouched.
for (agent, batch) in &priors {
let held = self.declared.get(agent).copied().unwrap_or_default();
if let (Some(existing), Some(new)) = (held.prior, batch.prior) {
if existing != new {
return Err(InferenceError::ConflictingCompetitorConfig {
competitor: agent.get(),
field: "prior",
});
}
}
if let (Some(existing), Some(new)) = (held.drift_scale, batch.drift_scale) {
if existing != new {
return Err(InferenceError::ConflictingCompetitorConfig {
competitor: agent.get(),
field: "drift_scale",
});
}
}
}
for (agent, batch) in &priors {
let entry = self.declared.entry(*agent).or_default();
if batch.prior.is_some() {
entry.prior = batch.prior;
}
if batch.drift_scale.is_some() {
entry.drift_scale = batch.drift_scale;
}
}
competitor::clean(self.agents.values_mut(), true); competitor::clean(self.agents.values_mut(), true);
let mut this_agent = Vec::with_capacity(1024); let mut this_agent = Vec::with_capacity(1024);
@@ -1658,7 +1812,9 @@ impl<T: Time, D: Drift<T>, O: Observer<T>, K: Eq + Hash + Clone> History<T, D, O
this_agent.push(*agent); this_agent.push(*agent);
let config = priors.get(agent).copied().unwrap_or_default(); // From `declared` rather than `priors`: a competitor configured by
// `register` before any event has nothing in this batch's map.
let config = self.declared.get(agent).copied().unwrap_or_default();
if self.agents.contains(*agent) { if self.agents.contains(*agent) {
// Seeding a competitor the history already knows. This used to // Seeding a competitor the history already knows. This used to
+338
View File
@@ -0,0 +1,338 @@
//! Configuring a competitor before anything is observed about them.
//!
//! The configuration a competitor needs is usually a property of the domain —
//! "every layout is static" — not of whichever event happens to mention them
//! first. Stating it per-event meant every ingestion path had to remember it,
//! and two of the four paths could not state it at all.
use smallvec::smallvec;
use trueskill_tt::{
ConstantDrift, ConvergenceOptions, Event, Gaussian, History, InferenceError, Member, Outcome,
Team,
};
type H = History<i64, ConstantDrift, trueskill_tt::NullObserver, &'static str>;
const PINNED: Gaussian = Gaussian::from_ms(2.0, 0.5);
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()
}
fn duel(
a: &'static str,
b: &'static str,
t: i64,
m: Option<Member<&'static str>>,
) -> Event<i64, &'static str> {
Event {
time: t,
teams: smallvec![
Team::with_members([Member::new(a)]),
Team::with_members([m.unwrap_or_else(|| Member::new(b))]),
],
outcome: Outcome::scores([5.0, 2.0]),
}
}
fn skills(h: &H) -> Vec<(&'static str, Gaussian)> {
["player", "layout"]
.into_iter()
.map(|k| (k, h.current_skill(&k).unwrap()))
.collect()
}
/// The headline contract.
#[test]
fn registering_matches_configuring_on_the_first_event() {
let configured = {
let mut h = history();
h.add_events(vec![
duel(
"player",
"layout",
1,
Some(
Member::new("layout")
.with_drift_scale(0.0)
.with_prior(PINNED),
),
),
duel("player", "layout", 2, None),
])
.unwrap();
let _ = h.converge().unwrap();
h
};
let registered = {
let mut h = history();
h.register(
Member::new("layout")
.with_drift_scale(0.0)
.with_prior(PINNED),
)
.unwrap();
h.add_events(vec![
duel("player", "layout", 1, None),
duel("player", "layout", 2, None),
])
.unwrap();
let _ = h.converge().unwrap();
h
};
for ((k, a), (_, b)) in skills(&configured).into_iter().zip(skills(&registered)) {
assert_eq!(a.pi(), b.pi(), "{k} pi");
assert_eq!(a.tau(), b.tau(), "{k} tau");
}
}
/// The case `EventBuilder` and the typed path cannot reach: a competitor whose
/// first appearance arrives through the two-argument convenience route.
#[test]
fn registration_reaches_a_competitor_first_seen_through_record_winner() {
let mut h = history();
h.register(
Member::new("layout")
.with_drift_scale(0.0)
.with_prior(PINNED),
)
.unwrap();
h.record_winner(&"player", &"layout", 1).unwrap();
h.record_winner(&"player", &"layout", 2).unwrap();
let _ = h.converge().unwrap();
let rating = h.rating(&"layout").unwrap();
assert_eq!(rating.drift_scale(), 0.0);
assert_eq!(rating.prior().mu(), PINNED.mu());
// Pinned means pinned: no drift across the two slices.
let curve = h.learning_curve(&"layout");
assert!(curve.len() >= 2);
let widest = curve
.iter()
.map(|(_, g)| g.sigma())
.fold(f64::MIN, f64::max);
let narrowest = curve
.iter()
.map(|(_, g)| g.sigma())
.fold(f64::MAX, f64::min);
assert!(
(widest - narrowest) / widest < 1e-9,
"{narrowest} .. {widest}"
);
}
#[test]
fn registering_a_known_competitor_is_an_error() {
let mut h = history();
h.record_winner(&"player", &"layout", 1).unwrap();
let err = h.register(Member::new("layout")).unwrap_err();
assert!(
matches!(err, InferenceError::AlreadyRegistered { .. }),
"{err:?}"
);
}
#[test]
fn registering_twice_is_an_error() {
let mut h = history();
h.register(Member::new("layout").with_drift_scale(0.0))
.unwrap();
let err = h
.register(Member::new("layout").with_drift_scale(1.0))
.unwrap_err();
assert!(
matches!(err, InferenceError::AlreadyRegistered { .. }),
"{err:?}"
);
// The first registration stands.
assert_eq!(h.rating(&"layout").unwrap().drift_scale(), 0.0);
}
/// `weight` is per-event and meaningless here, so it is rejected rather than
/// dropped — dropping it silently is the defect class this whole area keeps
/// producing.
#[test]
fn a_weight_on_a_registration_is_rejected() {
let mut h = history();
let err = h
.register(Member::new("layout").with_weight(0.5))
.unwrap_err();
assert!(
matches!(err, InferenceError::InvalidParameter { name: "weight", .. }),
"{err:?}"
);
}
#[test]
fn an_invalid_drift_scale_on_a_registration_is_rejected() {
for bad in [-1.0, f64::NAN, f64::INFINITY] {
let mut h = history();
let err = h
.register(Member::new("layout").with_drift_scale(bad))
.unwrap_err();
assert!(
matches!(
err,
InferenceError::InvalidParameter {
name: "drift_scale",
..
}
),
"{bad}: {err:?}"
);
}
}
/// Registration makes the fit independent of the order events arrive in,
/// which is what the per-event shape could not guarantee.
#[test]
fn registration_makes_the_fit_order_independent() {
let build = |reversed: bool| {
let mut h = history();
h.register(
Member::new("layout")
.with_drift_scale(0.0)
.with_prior(PINNED),
)
.unwrap();
let mut events = vec![
duel("player", "layout", 1, None),
duel("player", "layout", 2, None),
duel("player", "layout", 3, None),
];
if reversed {
events.reverse();
}
h.add_events(events).unwrap();
let _ = h.converge().unwrap();
h
};
let forward = build(false);
let backward = build(true);
for ((k, a), (_, b)) in skills(&forward).into_iter().zip(skills(&backward)) {
assert_eq!(a.pi(), b.pi(), "{k} pi");
assert_eq!(a.tau(), b.tau(), "{k} tau");
}
}
/// `rating` is the read-back that made a configuration mistake detectable from
/// outside the crate at all. Every other accessor reports what inference
/// inferred; this reports what it was told.
#[test]
fn rating_reads_back_what_was_stored() {
let mut h = history();
assert!(h.rating(&"nobody").is_none());
h.register(
Member::new("layout")
.with_drift_scale(0.25)
.with_prior(PINNED),
)
.unwrap();
let r = h.rating(&"layout").unwrap();
assert_eq!(r.drift_scale(), 0.25);
assert_eq!(r.prior().pi(), PINNED.pi());
assert_eq!(r.prior().tau(), PINNED.tau());
// A competitor created by an event reports the history defaults.
h.record_winner(&"player", &"layout", 1).unwrap();
assert_eq!(h.rating(&"player").unwrap().drift_scale(), 1.0);
}
/// The decision this issue turned on: two different values for one competitor
/// are an error whether they arrive in one batch or two.
///
/// Last-write-wins across batches cut against the invariant
/// `tests/ingestion_equivalence.rs` protects — the same contradictory events
/// errored when batched and succeeded, order-dependently, one at a time.
mod conflicting_configuration {
use super::*;
fn seed(scale: f64) -> Event<i64, &'static str> {
duel(
"player",
"layout",
1,
Some(Member::new("layout").with_drift_scale(scale)),
)
}
#[test]
fn within_one_batch_is_an_error() {
let mut h = history();
let err = h.add_events(vec![seed(0.0), seed(1.0)]).unwrap_err();
assert!(
matches!(
err,
InferenceError::ConflictingCompetitorConfig {
field: "drift_scale",
..
}
),
"{err:?}"
);
}
#[test]
fn across_two_batches_is_also_an_error() {
let mut h = history();
h.add_events(vec![seed(0.0)]).unwrap();
let err = h.add_events(vec![seed(1.0)]).unwrap_err();
assert!(
matches!(
err,
InferenceError::ConflictingCompetitorConfig {
field: "drift_scale",
..
}
),
"{err:?}"
);
// Rejected before anything mutates: the first declaration stands.
assert_eq!(h.rating(&"layout").unwrap().drift_scale(), 0.0);
}
/// Repeating the *same* value stays inert, which is the expected shape
/// when the configuration is a property of the domain.
#[test]
fn repeating_the_same_value_is_inert() {
let mut h = history();
h.add_events(vec![seed(0.0)]).unwrap();
h.add_events(vec![seed(0.0)]).unwrap();
assert_eq!(h.rating(&"layout").unwrap().drift_scale(), 0.0);
}
/// A registration and a later event that agree are fine; one that
/// disagrees is the same error.
#[test]
fn a_registration_conflicts_with_a_later_event() {
let mut h = history();
h.register(Member::new("layout").with_drift_scale(0.0))
.unwrap();
h.add_events(vec![seed(0.0)]).unwrap();
let mut h2 = history();
h2.register(Member::new("layout").with_drift_scale(0.0))
.unwrap();
let err = h2.add_events(vec![seed(1.0)]).unwrap_err();
assert!(
matches!(err, InferenceError::ConflictingCompetitorConfig { .. }),
"{err:?}"
);
}
}