5 Commits
Author SHA1 Message Date
logaritmiskandClaude Opus 5 862779ae34 Merge branch 'feat/convergence-strictness'
Make a short fit an error, raise the default iteration cap, validate the
remaining HistoryBuilder parameters, add History::register and
History::rating, reject competitor config conflicts across batches, and
document what the joint's cost scales in.

Closes #50

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011hcFjNDmHXZF8URGLku5zZ
2026-09-08 21:00:39 +02:00
logaritmiskandClaude Opus 5 f692906ce4 docs: state what the joint's cost actually scales in
A consumer measured an 8x difference in solve time between two fits over
the same events, the same slices and the same ~2,000 nodes:

  career fit   (gamma = 0)      787 ms
  drifting fit (gamma = 0.15)  6214 ms

Entirely the collapse rule. A competitor with zero drift contributes one
variable however long the history, so a drift-free fit's joint is smaller
than a drifting one's by roughly the slice count — and to factorise, by
its cube. Choosing a drift configuration is therefore also choosing a
query cost, and nothing said so.

Documented on `Joint`, on `Joint::variables` and on `posterior_of`, with
the measurement. `variables()` is named as the number that decides
affordability, since it can be read before committing to a batch.

Also states the thing the consumer proposed as a future optimisation,
because it is already true: an absence is not an appearance, so a
competitor seen in the first and last of a hundred slices contributes two
variables rather than a hundred. The matrix is already as small as the
model allows on that axis.

tests/joint_handle.rs pins the mechanism — ten slices, two competitors,
twenty variables drifting against two at `gamma = 0` — so a change to the
collapse rule cannot quietly remove the property the docs now promise.

Refs #51

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011hcFjNDmHXZF8URGLku5zZ
2026-09-08 19:45:51 +02:00
logaritmiskandClaude Opus 5 e493f47e99 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
2026-09-08 16:11:09 +02:00
logaritmiskandClaude Opus 5 4f6360128d feat!: validate mu, sigma and beta on HistoryBuilder
The last three unvalidated setters, beside `p_draw`, `score_sigma` and
`convergence`, which all assert eagerly. Measured before choosing bounds:

  beta = 0        -> works, pi 0.0211 (vs 0.0193 at the default)
  beta = -4.17    -> bit-identical to +4.17
  sigma = -8.33   -> bit-identical to +8.33
  sigma = 0       -> NonFiniteResult, current_skill returns tau: NaN
  sigma = inf     -> same
  mu = NaN        -> same

So the bounds are not the obvious ones. `beta = 0` is legitimate and
meaningful — performance is then exactly skill, and the fit moves
measurably rather than degenerating — so zero is allowed and a test pins
that it reaches a different answer, since "allowed" would otherwise be
indistinguishable from "unchecked".

The negative cases are the quiet ones. `sigma` and `beta` enter inference
only as squares, so a negative value behaves as its absolute value and
the sign is dropped without comment. That is the same defect
`Member::with_drift_scale` already rejects, for the reason already
written there.

The non-finite cases are detected today — `converge` reports
NonFiniteResult — but a caller who reads `current_skill` first is handed
`tau: NaN`, so rejecting at the boundary is what actually closes it.

BREAKING CHANGE: `HistoryBuilder::mu`, `sigma` and `beta` now panic on
values they previously accepted, matching the existing behaviour of
`p_draw`, `score_sigma` and `convergence`.

Refs #18

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011hcFjNDmHXZF8URGLku5zZ
2026-09-08 16:06:37 +02:00
logaritmiskandClaude Opus 5 eff63dfa2a feat!: make a short fit an error and raise the default iteration cap
`ITERATIONS` was 30, and overrunning it returned `Ok` with
`converged: false`. Both halves were wrong.

The cap is a runaway guard, not a budget: the sweep exits as soon as the
step falls below `epsilon`, so a high cap costs nothing on a history that
converges. Measured on one needing four sweeps, `max_iter` 30 and
100_000 both finish in 4 iterations and ~130us. So 30 could never make
anything faster — it could only stop a healthy history early, and it did:
160 events over 100 competitors already needs 42.

Not scaled to the history, because iteration count tracks how loopy the
graph is rather than how big it is. At a fixed 320 events over 40 slices,
varying only the competitors sharing them: 3 competitors needs 2_789
sweeps, 10 needs 1_068, 100 needs 90, 400 needs 2. Three orders of
magnitude on identical event and slice counts, so any formula in those
two numbers would be badly wrong on some real shape. A single value set
high enough that reaching it means oscillation is the honest version.

With the cap raised, stopping at it means something is genuinely wrong,
so `converge` now returns `InferenceError::NotConverged` rather than a
flag on a success. A short fit is wrong by a little — every rating
finite, the ordering sensible, nothing saying the numbers were still
moving — and a flag has to be checked while `let _ = h.converge()` is the
natural way not to. That is not hypothetical: it is how a real defect hid
in this crate's own test suite.

`converge_partial` returns the short fit for callers who want one. Only a
single existing test needed it, which is the evidence that a capped fit
is a deliberate choice rather than the common case.

Also corrects the `ITERATIONS` docs, which claimed convergence cost is
"roughly linear in the cap". It is linear in the iterations actually run.

BREAKING CHANGE: `History::converge` returns `Err(NotConverged)` where it
previously returned `Ok` with `converged: false`. Callers that want the
old behaviour should use `History::converge_partial`. The default
`max_iter` changes from 30 to 10_000, so a history that was silently
truncated will now converge properly and its numbers will move.

Closes #50

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011hcFjNDmHXZF8URGLku5zZ
2026-09-08 16:04:22 +02:00
8 changed files with 996 additions and 27 deletions
+10 -3
View File
@@ -62,10 +62,17 @@ impl Default for ConvergenceOptions {
}
/// Post-hoc summary of a `History::converge` call.
///
/// From [`History::converge`](crate::History::converge) this always describes a
/// converged fit — stopping at `max_iter` is
/// [`InferenceError::NotConverged`](crate::InferenceError::NotConverged) there.
/// From [`History::converge_partial`](crate::History::converge_partial) it may
/// not be, and `converged` is what says so.
#[derive(Clone, Debug)]
#[must_use = "a ConvergenceReport carries `converged`, and a fit that stopped \
at `max_iter` is wrong by a little rather than loudly broken — \
check it, or bind it to `_` to say you have decided not to"]
#[must_use = "from `converge_partial` this may describe a fit that stopped at \
`max_iter`, which is wrong by a little rather than loudly \
broken — check `converged`, or bind it to `_` to say you have \
decided not to"]
pub struct ConvergenceReport {
pub iterations: usize,
pub final_step: (f64, f64),
+49
View File
@@ -64,6 +64,24 @@ pub enum InferenceError {
/// result has no representable likelihood. Configure a positive `p_draw`
/// (via `HistoryBuilder::p_draw` or `GameOptions::p_draw`) to admit ties.
TieWithoutDrawProbability { teams: (usize, usize) },
/// The convergence sweep hit `max_iter` with the step still above
/// `epsilon`.
///
/// A fit that stops short is wrong by a little, which is the worst
/// available failure: every rating is finite, the ordering looks sensible,
/// and nothing in the numbers says they were still moving. Reported rather
/// than returned as a flag on an `Ok`, because a flag has to be checked
/// and `let _ = h.converge()` is the natural way not to.
///
/// Either the history needs more iterations — raise `max_iter` — or it is
/// oscillating rather than converging, in which case `alpha < 1.0` damps
/// the within-game EP loop. [`History::converge_partial`](crate::History::converge_partial)
/// returns the short fit instead when that is genuinely what is wanted.
NotConverged {
iterations: usize,
final_step: (f64, f64),
epsilon: f64,
},
/// Inference produced a non-finite value (NaN or infinity).
///
/// Indicates numerical breakdown; the resulting skills are meaningless
@@ -100,6 +118,17 @@ pub enum InferenceError {
member: usize,
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.
EmptyTeam { team: usize },
/// A joint posterior was requested where one cannot be formed exactly.
@@ -144,6 +173,18 @@ impl fmt::Display for InferenceError {
teams.0, teams.1
)
}
Self::NotConverged {
iterations,
final_step,
epsilon,
} => {
write!(
f,
"did not converge in {iterations} iterations: final step {final_step:?} \
is still above epsilon {epsilon}; raise max_iter, or damp with \
alpha < 1.0 if it is oscillating"
)
}
Self::NonFiniteResult { context, step } => {
write!(
f,
@@ -167,6 +208,14 @@ impl fmt::Display for InferenceError {
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 } => {
write!(f, "team {team} has no members")
}
+294 -10
View File
@@ -6,6 +6,7 @@ use crate::{
convergence::{ConvergenceOptions, ConvergenceReport},
drift::{ConstantDrift, Drift},
error::InferenceError,
event::Member,
gaussian::Gaussian,
key_table::KeyTable,
observer::{NullObserver, Observer},
@@ -39,17 +40,55 @@ pub struct HistoryBuilder<
}
impl<T: Time, D: Drift<T>, O: Observer<T>, K: Eq + Hash + Clone> HistoryBuilder<T, D, O, K> {
/// Prior mean skill.
///
/// # Panics
///
/// Panics if `mu` is not finite. A non-finite prior mean poisons every
/// posterior derived from it: `converge` reports `NonFiniteResult`, but a
/// caller who reads `current_skill` first is handed `tau: NaN`.
pub fn mu(mut self, mu: f64) -> Self {
assert!(mu.is_finite(), "mu must be finite (got {mu})");
self.mu = mu;
self
}
/// Prior standard deviation.
///
/// # Panics
///
/// Panics unless `sigma` is finite and strictly positive.
///
/// Zero and infinity both give a prior precision that is not a number, and
/// the whole fit comes back NaN. A *negative* sigma is the quieter half:
/// it is only ever squared, so `-8.33` produces bit-identical results to
/// `8.33` — a sign the caller cannot have meant, silently ignored.
pub fn sigma(mut self, sigma: f64) -> Self {
assert!(
sigma.is_finite() && sigma > 0.0,
"sigma must be finite and positive (got {sigma})"
);
self.sigma = sigma;
self
}
/// Per-event performance noise.
///
/// # Panics
///
/// Panics unless `beta` is finite and non-negative.
///
/// Zero is allowed and meaningful — performance is then exactly skill, and
/// the fit differs measurably from a positive `beta` rather than
/// degenerating. Negative is rejected for the same reason as a negative
/// `sigma` or `Member::with_drift_scale`: `beta` enters only as `beta^2`,
/// so a negative value behaves as its absolute value and the sign is lost
/// without comment.
pub fn beta(mut self, beta: f64) -> Self {
assert!(
beta.is_finite() && beta >= 0.0,
"beta must be finite and non-negative (got {beta})"
);
self.beta = beta;
self
}
@@ -169,6 +208,7 @@ impl<T: Time, D: Drift<T>, O: Observer<T>, K: Eq + Hash + Clone> HistoryBuilder<
convergence: self.convergence,
observer: self.observer,
unknown_keys: self.unknown_keys,
declared: HashMap::new(),
}
}
}
@@ -276,6 +316,12 @@ pub struct History<
convergence: ConvergenceOptions,
observer: O,
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> {
@@ -455,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.
/// 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>
where
K: std::borrow::Borrow<Q>,
@@ -958,6 +1109,14 @@ impl<T: Time, D: Drift<T>, O: Observer<T>, K: Eq + Hash + Clone> History<T, D, O
/// [`Joint`] with [`History::joint`] instead — the answers are identical,
/// and only the first one pays.
///
/// # Cost
///
/// A dense solve over the history's *appearances*, not its competitors. A
/// drift-free competitor collapses to a single variable however long the
/// history, so the same events can differ enormously in cost depending on
/// the drift configuration — see [`Joint`], which also amortises this
/// across many questions.
///
/// # Limitations
///
/// Exact only for a history whose events are all scored, because a scored
@@ -1390,17 +1549,62 @@ impl<T: Time, D: Drift<T>, O: Observer<T>, K: Eq + Hash + Clone> History<T, D, O
))
}
/// Run the full forward+backward convergence loop and return a summary.
/// Run the full forward+backward convergence loop to a fixed point.
///
/// Failing to reach `epsilon` within `max_iter` is not an error: the
/// returned report carries `converged: false` and the final step.
/// # Stopping short is an error
///
/// Hitting `max_iter` without reaching `epsilon` returns `NotConverged`.
///
/// It used to return `Ok` with `converged: false`, which was the worst
/// available shape. A fit that stops short is *wrong by a little*: every
/// rating is finite, the ordering looks sensible, and nothing about the
/// output says the numbers were still moving. Detection was opt-in, and
/// `let _ = h.converge()` silently opted out — which is how a real defect
/// hid in this crate's own test suite.
///
/// The default `max_iter` is [`ITERATIONS`](crate::ITERATIONS), which is
/// set high enough that reaching it means something is genuinely wrong
/// rather than that the history is merely large. Raising the cap costs
/// nothing when it is not needed, because the loop exits at `epsilon`.
///
/// Use [`History::converge_partial`] when a capped, unconverged fit is
/// what you actually want.
///
/// # Errors
///
/// `NotConverged` if the sweep hits `max_iter` with the step still above
/// `epsilon`.
///
/// `NonFiniteResult` if a sweep produces a NaN or infinite step. EP has
/// broken down at that point and further iterations cannot recover, so the
/// loop stops rather than reporting a NaN step as convergence.
pub fn converge(&mut self) -> Result<ConvergenceReport, InferenceError> {
let report = self.converge_partial()?;
if report.converged {
Ok(report)
} else {
Err(InferenceError::NotConverged {
iterations: report.iterations,
final_step: report.final_step,
epsilon: self.convergence.epsilon,
})
}
}
/// As [`History::converge`], but a fit that stops at `max_iter` is
/// returned rather than reported as an error.
///
/// The report's `converged` flag says which happened. Use this when a
/// deliberately capped sweep is the point — a cheap approximate fit, or a
/// test that pins what a fixed number of iterations produces. Prefer
/// `converge` everywhere else: an unconverged fit that nobody checks is
/// indistinguishable from a converged one.
///
/// # Errors
///
/// `NonFiniteResult` if a sweep produces a NaN or infinite step.
pub fn converge_partial(&mut self) -> Result<ConvergenceReport, InferenceError> {
use std::time::Instant;
use smallvec::SmallVec;
@@ -1564,6 +1768,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);
let mut this_agent = Vec::with_capacity(1024);
@@ -1575,7 +1820,9 @@ impl<T: Time, D: Drift<T>, O: Observer<T>, K: Eq + Hash + Clone> History<T, D, O
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) {
// Seeding a competitor the history already knows. This used to
@@ -1966,6 +2213,34 @@ impl<T: Time, D: Drift<T>, O: Observer<T>, K: Eq + Hash + Clone> History<T, D, O
/// added while it is alive. That is what makes it correct without any
/// invalidation logic: there is no window in which the factorisation could
/// describe a fit that no longer exists.
///
/// # What the cost actually scales in
///
/// Not competitors, and not slices times competitors. One variable per
/// *appearance* — a competitor per slice they appear in — minus every
/// consecutive pair with no drift between them, which collapse to a single
/// latent variable.
///
/// That last clause dominates, and it is not obvious. A competitor whose drift
/// is zero contributes **one** variable however long the history: whole-history
/// `gamma = 0`, or `drift_scale = 0` on that competitor. So two fits over the
/// same events and the same slices can differ in problem size by roughly the
/// slice count, and in factorisation time by its cube. Measured by a consumer
/// on a ~2,000-node model over 76 slices:
///
/// ```text
/// career fit (gamma = 0) 787 ms per solve
/// drifting fit (gamma = 0.15) 6214 ms per solve
/// ```
///
/// Choosing between a drifting and a drift-free configuration is therefore also
/// choosing an 8x difference in query cost. [`Joint::variables`] reports the
/// number that decides it, and can be read before committing to a batch of
/// queries.
///
/// Slices a competitor sits out cost nothing: an absence is not an appearance,
/// so a competitor seen in the first and last of a hundred slices contributes
/// two variables, not a hundred.
pub struct Joint<'h, T: Time, D: Drift<T>, O: Observer<T>, K: Eq + Hash + Clone> {
history: &'h History<T, D, O, K>,
cholesky: crate::joint::Cholesky,
@@ -1993,9 +2268,14 @@ impl<T: Time, D: Drift<T>, O: Observer<T>, K: Eq + Hash + Clone> Joint<'_, T, D,
/// Number of variables in the joint: the history's appearances, after
/// collapsing consecutive pairs a competitor does not drift between.
///
/// This is what the cost scales in, and it is not the competitor count — a
/// competitor contributes one variable per slice it appears in. Worth
/// checking before asking for a joint over a long history.
/// This is what the cost scales in — `O(n^3)` to factorise, `O(n^2)` per
/// query — and it is neither the competitor count nor slices times
/// competitors. A drift-free competitor contributes one variable however
/// many slices they appear in; see the type docs for how large that
/// difference gets.
///
/// Worth reading before committing to a batch of queries: it is the one
/// number that says whether a joint over this history is affordable.
#[must_use]
pub fn variables(&self) -> usize {
self.width
@@ -2799,13 +3079,15 @@ mod tests {
epsilon = 1e-6
);
// run exactly 11 iterations (old test used convergence(11, ...))
// Run exactly 11 iterations. `converge_partial` rather than
// `converge`: stopping at the cap is the point here, and `converge`
// now reports that as `NotConverged`.
h.convergence = ConvergenceOptions {
max_iter: 11,
epsilon: EPSILON,
alpha: 1.0,
};
let _ = h.converge().unwrap();
let _ = h.converge_partial().unwrap();
let loocv_approx_2 = h.log_evidence_internal(false, &[]).exp().sqrt();
@@ -3172,7 +3454,9 @@ mod tests {
})
.build();
events_for(&mut h_capped);
let _ = h_capped.converge().unwrap();
// A one-iteration cap is deliberate here, so the short fit is the
// result rather than an error.
let _ = h_capped.converge_partial().unwrap();
let mut h_full: History<i64, _, _, &'static str> = History::builder().build();
events_for(&mut h_full);
+35 -14
View File
@@ -158,22 +158,43 @@ pub const P_DRAW: f64 = 0.0;
pub const EPSILON: f64 = 1e-6;
/// Default cap on convergence sweeps.
///
/// **This is a floor, not a recommendation.** It is adequate for small
/// histories and is quickly outgrown: a history of 400 events over 100
/// competitors already stops here with a final step of ~7e-3 against the 1e-6
/// default tolerance — four orders of magnitude short — and a dense joint model
/// of ~2,000 nodes over ~3,300 events has been measured needing 76 to 161.
/// **A runaway guard, not a budget.** The sweep exits as soon as the step falls
/// below `epsilon`, so the cap is never reached by a history that converges and
/// raising it costs nothing. Measured on a history that needs four sweeps:
///
/// Overrunning it is not an error, and deliberately so: `converge` returns a
/// [`ConvergenceReport`] whose `converged` flag says what happened. But a fit
/// that stopped short is *wrong by a little*, which is the worst available
/// failure — every rating is finite and ordered sensibly, and nothing in the
/// numbers themselves says they were still moving. Read the report; the type is
/// `#[must_use]` for that reason.
/// ```text
/// max_iter 30: 4 iterations, 129.9 us
/// max_iter 100_000: 4 iterations, 131.9 us
/// ```
///
/// Raise it via [`ConvergenceOptions`]. Convergence cost is roughly linear in
/// the cap, and for anything but a toy the extra sweeps are milliseconds.
pub const ITERATIONS: usize = 30;
/// This was `30` until it was measured, and 30 truncated ordinary healthy
/// histories: 160 events over 100 competitors already needs 42. Because a short
/// fit is finite and sensibly ordered, that was invisible.
///
/// # Why it is not scaled to the history
///
/// The obvious improvement — pick the cap from the node or event count — does
/// not work, because iteration count is driven by how *loopy* the graph is
/// rather than how big it is. At a fixed 320 events over 40 slices, varying
/// only the number of competitors sharing them:
///
/// ```text
/// competitors appearances each iterations
/// 3 213 2_789
/// 10 64 1_068
/// 50 12.8 206
/// 100 6.4 90
/// 400 1.6 2
/// ```
///
/// Three orders of magnitude apart on identical event and slice counts. Any
/// formula in those two numbers would be badly wrong on some real shape, so the
/// cap is a single value set high enough that reaching it means the fit is
/// oscillating rather than merely large.
///
/// Reaching it is [`InferenceError::NotConverged`]. See
/// [`History::converge`](crate::History::converge).
pub const ITERATIONS: usize = 10_000;
/// Largest team count `History::predict_outcome` will enumerate.
///
+152
View File
@@ -0,0 +1,152 @@
//! Stopping short of convergence is an error, not a flag on a success.
//!
//! A fit that hits `max_iter` is wrong by a little: every rating is finite,
//! the ordering looks sensible, and nothing in the numbers says they were
//! still moving. When that was `Ok` with `converged: false`, detecting it was
//! opt-in and `let _ = h.converge()` was the natural way to opt out — which is
//! how a real defect once hid in this crate's own suite.
use smallvec::smallvec;
use trueskill_tt::{
ConstantDrift, ConvergenceOptions, Event, History, InferenceError, Member, Outcome, Team,
};
type H = History<i64, ConstantDrift, trueskill_tt::NullObserver, &'static str>;
fn duel(a: &'static str, b: &'static str, t: i64) -> Event<i64, &'static str> {
Event {
time: t,
teams: smallvec![
Team::with_members([Member::new(a)]),
Team::with_members([Member::new(b)]),
],
outcome: Outcome::scores([3.0, 1.0]),
}
}
fn capped(max_iter: usize) -> H {
History::builder()
.mu(0.0)
.sigma(6.0)
.beta(1.0)
.score_sigma(2.0)
.drift(ConstantDrift(0.5))
.convergence(ConvergenceOptions {
max_iter,
epsilon: 1e-13,
alpha: 1.0,
})
.build()
}
fn fill(h: &mut H) {
h.add_events((1..=6).map(|t| duel("a", "b", t)).collect::<Vec<_>>())
.unwrap();
}
#[test]
fn hitting_the_cap_is_an_error() {
let mut h = capped(1);
fill(&mut h);
let err = h.converge().unwrap_err();
match err {
InferenceError::NotConverged {
iterations,
final_step,
epsilon,
} => {
assert_eq!(iterations, 1);
assert!(
final_step.0 > epsilon || final_step.1 > epsilon,
"{final_step:?}"
);
}
other => panic!("expected NotConverged, got {other:?}"),
}
}
/// The message has to name what to do about it, since the fit looks fine.
#[test]
fn the_error_says_how_to_fix_it() {
let mut h = capped(1);
fill(&mut h);
let text = h.converge().unwrap_err().to_string();
assert!(text.contains("did not converge in 1 iterations"), "{text}");
assert!(text.contains("max_iter"), "{text}");
assert!(text.contains("alpha"), "{text}");
}
/// The escape hatch: a deliberately capped fit is still reachable.
#[test]
fn converge_partial_returns_the_short_fit() {
let mut h = capped(1);
fill(&mut h);
let report = h.converge_partial().unwrap();
assert_eq!(report.iterations, 1);
assert!(!report.converged);
assert!(h.current_skill(&"a").is_some());
}
/// Both agree when the fit does converge, so the strict path costs nothing.
#[test]
fn the_two_agree_on_a_converged_fit() {
let mut strict = capped(20_000);
fill(&mut strict);
let a = strict.converge().unwrap();
let mut partial = capped(20_000);
fill(&mut partial);
let b = partial.converge_partial().unwrap();
assert!(a.converged && b.converged);
assert_eq!(a.iterations, b.iterations);
assert_eq!(a.final_step, b.final_step);
}
/// The default cap must be high enough that an ordinary history clears it.
/// 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<i64, ConstantDrift, _, String> = History::builder_with_key()
.mu(0.0)
.sigma(6.0)
.beta(1.0)
.score_sigma(2.0)
.drift(ConstantDrift(0.05))
.build();
let mut events = Vec::new();
for t in 0..20i64 {
for j in 0..8usize {
let k = (t as usize) * 8 + j;
events.push(Event {
time: t,
teams: smallvec![
Team::with_members([Member::new(format!("p{}", k % 100))]),
Team::with_members([Member::new(format!("p{}", (k + 37) % 100))]),
],
outcome: Outcome::scores([3.0, 1.0]),
});
}
}
h.add_events(events).unwrap();
let report = h
.converge()
.expect("an ordinary history must converge by default");
assert!(
report.iterations > 30,
"needed {} sweeps",
report.iterations
);
assert!(report.iterations < trueskill_tt::ITERATIONS);
}
/// An empty history converges trivially rather than erroring.
#[test]
fn an_empty_history_converges() {
let mut h = capped(1);
let report = h.converge().unwrap();
assert!(report.converged);
assert_eq!(report.iterations, 0);
}
+47
View File
@@ -141,6 +141,53 @@ fn variables_counts_appearances_not_competitors() {
assert_eq!(joint.variables(), 12);
}
/// How much the collapse is worth, which is the part a caller has to plan
/// around: a drift-free competitor contributes **one** variable however long
/// the history, so the same events at `gamma = 0` and `gamma > 0` differ by
/// roughly the slice count in problem size — and by its cube in solve time.
///
/// Reported by a consumer as an 8x difference in solve time on a ~2,000-node,
/// 76-slice model (787 ms career against 6,214 ms drifting). This pins the
/// mechanism behind that so a change to the collapse rule cannot quietly
/// remove it.
#[test]
fn drift_free_competitors_shrink_the_joint_by_the_slice_count() {
fn variables(gamma: f64) -> usize {
let mut h = History::builder()
.mu(0.0)
.sigma(6.0)
.beta(1.0)
.score_sigma(2.0)
.drift(ConstantDrift(gamma))
.convergence(ConvergenceOptions {
max_iter: 20_000,
epsilon: 1e-13,
alpha: 1.0,
})
.build();
h.add_events(
(1..=10)
.map(|t| duel("a", "b", t, 5.0, 2.0))
.collect::<Vec<_>>(),
)
.unwrap();
let _ = h.converge().unwrap();
h.joint().unwrap().variables()
}
let drifting = variables(0.5);
let career = variables(0.0);
// Two competitors over ten slices: twenty appearances, or two variables.
assert_eq!(drifting, 20);
assert_eq!(career, 2);
assert_eq!(
drifting / career,
10,
"collapse should track the slice count"
);
}
/// With `drift = 0` consecutive appearances are the same latent variable, so
/// the joint is smaller than the appearance count.
#[test]
+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:?}"
);
}
}
+71
View File
@@ -184,3 +184,74 @@ fn ingestion_rejects_weights_that_do_not_match_their_team() {
"got {err:?}"
);
}
/// `mu`, `sigma` and `beta` were the last unvalidated setters on
/// `HistoryBuilder`, next to `p_draw`, `score_sigma` and `convergence`, which
/// all assert eagerly.
///
/// Two of the rejected values are the quiet kind. A negative `sigma` or `beta`
/// enters inference only as its square, so it produced bit-identical results
/// to the positive value — the sign was dropped without comment.
mod builder_parameters {
use trueskill_tt::History;
#[test]
#[should_panic(expected = "mu must be finite")]
fn a_non_finite_mu_is_rejected() {
let _ = History::builder().mu(f64::NAN);
}
#[test]
#[should_panic(expected = "sigma must be finite and positive")]
fn a_zero_sigma_is_rejected() {
let _ = History::builder().sigma(0.0);
}
#[test]
#[should_panic(expected = "sigma must be finite and positive")]
fn a_negative_sigma_is_rejected() {
let _ = History::builder().sigma(-8.33);
}
#[test]
#[should_panic(expected = "sigma must be finite and positive")]
fn an_infinite_sigma_is_rejected() {
let _ = History::builder().sigma(f64::INFINITY);
}
#[test]
#[should_panic(expected = "beta must be finite and non-negative")]
fn a_negative_beta_is_rejected() {
let _ = History::builder().beta(-4.17);
}
#[test]
#[should_panic(expected = "beta must be finite and non-negative")]
fn a_non_finite_beta_is_rejected() {
let _ = History::builder().beta(f64::NAN);
}
/// Zero beta is deliberately allowed: performance is then exactly skill.
/// It has to reach a different fit than a positive beta, or "allowed"
/// would just mean "not checked".
#[test]
fn a_zero_beta_is_allowed_and_changes_the_fit() {
let fit = |beta: f64| {
let mut h = History::builder()
.mu(25.0)
.sigma(25.0 / 3.0)
.beta(beta)
.build();
h.record_winner(&"a", &"b", 1).unwrap();
let _ = h.converge().unwrap();
h.current_skill(&"a").unwrap()
};
let zero = fit(0.0);
let positive = fit(25.0 / 6.0);
assert!(zero.pi().is_finite() && zero.pi() > 0.0);
assert!(
(zero.pi() - positive.pi()).abs() > 1e-6,
"zero beta must not merely be ignored: {zero:?} vs {positive:?}"
);
}
}