2 Commits
Author SHA1 Message Date
logaritmisk 6e2ce69728 Merge api/retire-index (#73) 2026-09-09 23:18:54 +02:00
logaritmiskandClaude Opus 5 faa25fb3b1 refactor!: retire Index, intern and lookup
`Index` was public, `History::intern` and `History::lookup` returned
one, and no public method anywhere accepted one. It was a handle with
nowhere to go — and `key_table.rs` advertised the hot-path story it was
meant to enable ("power users can promote `&K` to `Index` and skip the
lookup"), which was never reachable through the public API.

It also shadowed `std::ops::Index`, which `CompetitorStore` implements,
so `use trueskill_tt::*` alongside `use std::ops::*` collided.

All three are `pub(crate)` now. `intern` stays internal because
ingestion needs it; `lookup` is gone entirely, since `current_skill`,
`rating` and `learning_curve` already answer "does this history know
this key" and all three take a borrowed key.

The three tests that used them asserted things a caller cannot observe.
They now assert what the interning bought:

- `record_winner_creates_two_competitors` compares posteriors instead of
  comparing two opaque indices for inequality.
- `intern_is_idempotent` becomes `a_repeated_key_is_one_competitor` — a
  key appearing in two events gives one competitor with a two-point
  learning curve, which is the observable form of the same claim.
- `lookup_returns_none_for_missing` becomes `an_unknown_key_is_unknown`.

Closes #73.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011hcFjNDmHXZF8URGLku5zZ
2026-09-09 23:18:54 +02:00
7 changed files with 57 additions and 62 deletions
+1 -1
View File
@@ -330,7 +330,7 @@ stay available at any size:
Unknown keys are an error by default, not a silent omission: a team the history Unknown keys are an error by default, not a silent omission: a team the history
has never seen cannot produce a confident-looking probability. The error names has never seen cannot produce a confident-looking probability. The error names
the key, and every key must already be known — pre-filter with `lookup` or the key, and every key must already be known — pre-filter with
`current_skill` if your caller cannot guarantee that. `current_skill` if your caller cannot guarantee that.
If predicting for competitors you have never seen is the point rather than a If predicting for competitors you have never seen is the point rather than a
+2 -2
View File
@@ -155,7 +155,7 @@ pub enum InferenceError {
/// when a competitor's configuration is a property of the domain. /// when a competitor's configuration is a property of the domain.
#[non_exhaustive] #[non_exhaustive]
ConflictingCompetitorConfig { ConflictingCompetitorConfig {
/// The competitor's interned [`Index`](crate::Index) as a raw `usize`, /// The competitor's interned slot as a raw `usize`,
/// not the user key — the batch is already flattened to indices by the /// not the user key — the batch is already flattened to indices by the
/// time the conflict is detectable. /// time the conflict is detectable.
competitor: usize, competitor: usize,
@@ -323,7 +323,7 @@ impl fmt::Display for InferenceError {
f, f,
"team {team}, member {member}: no skill recorded for key {key} \ "team {team}, member {member}: no skill recorded for key {key} \
(every key must already be known to the history; pre-filter \ (every key must already be known to the history; pre-filter \
with `lookup` or `current_skill` if that is not guaranteed)" with `current_skill` if that is not guaranteed)"
) )
} }
Self::AlreadyRegistered { key } => { Self::AlreadyRegistered { key } => {
+9 -22
View File
@@ -538,29 +538,16 @@ 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<T, D, O, K> {
/// Promote a key to its [`Index`], creating the entry if it is new. /// Promote a key to its [`Index`], creating the entry if it is new.
/// ///
/// Interning a key does not register a competitor or give them a rating /// Crate-internal since #73: interning reserves a storage slot and nothing
/// it only reserves the slot. Use [`History::register`] to declare a /// more, and no public method ever accepted the handle it returned.
/// competitor's configuration up front. /// [`History::register`] is the public way to declare a competitor.
pub fn intern<Q>(&mut self, key: &Q) -> Index pub(crate) fn intern<Q>(&mut self, key: &Q) -> Index
where where
K: Borrow<Q>, K: Borrow<Q>,
Q: Hash + Eq + ToOwned<Owned = K> + ?Sized, Q: Hash + Eq + ToOwned<Owned = K> + ?Sized,
{ {
self.keys.get_or_create(key) self.keys.get_or_create(key)
} }
/// Resolve an existing key to its [`Index`], or `None` if the history has
/// never seen it.
///
/// The read-only counterpart of [`History::intern`]: it never creates.
#[must_use]
pub fn lookup<Q>(&self, key: &Q) -> Option<Index>
where
K: Borrow<Q>,
Q: Hash + Eq + ?Sized,
{
self.keys.get(key)
}
} }
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<T, D, O, K> {
@@ -1328,7 +1315,7 @@ impl<T: Time, D: Drift<T>, O: Observer<T>, K: Eq + Hash + Clone> History<T, D, O
/// Every key must already be known to the history — that is, must have /// Every key must already be known to the history — that is, must have
/// appeared in an ingested event. An unknown key is `UnknownKey`, not a /// appeared in an ingested event. An unknown key is `UnknownKey`, not a
/// silently dropped member. If your caller cannot guarantee that, pre-filter /// silently dropped member. If your caller cannot guarantee that, pre-filter
/// with [`History::lookup`] or [`History::current_skill`]; treating the /// with [`History::current_skill`] or [`History::competitors`]; treating the
/// error as "no information" and substituting a neutral value turns a /// error as "no information" and substituting a neutral value turns a
/// whole-team miss into a plausible constant, which is invisible to any /// whole-team miss into a plausible constant, which is invisible to any
/// test that does not assert on variation. /// test that does not assert on variation.
@@ -1849,7 +1836,7 @@ impl<T: Time, D: Drift<T>, O: Observer<T>, K: Eq + Hash + Clone> History<T, D, O
/// Every key must already be known to the history — that is, must have /// Every key must already be known to the history — that is, must have
/// appeared in an ingested event. An unknown key is `UnknownKey`, not a /// appeared in an ingested event. An unknown key is `UnknownKey`, not a
/// silently dropped member. If your caller cannot guarantee that, pre-filter /// silently dropped member. If your caller cannot guarantee that, pre-filter
/// with [`History::lookup`] or [`History::current_skill`]; treating the /// with [`History::current_skill`] or [`History::competitors`]; treating the
/// error as "no information" and substituting a neutral value turns a /// error as "no information" and substituting a neutral value turns a
/// whole-team miss into a plausible constant, which is invisible to any /// whole-team miss into a plausible constant, which is invisible to any
/// test that does not assert on variation. /// test that does not assert on variation.
@@ -1913,7 +1900,7 @@ impl<T: Time, D: Drift<T>, O: Observer<T>, K: Eq + Hash + Clone> History<T, D, O
/// Every key must already be known to the history — that is, must have /// Every key must already be known to the history — that is, must have
/// appeared in an ingested event. An unknown key is `UnknownKey`, not a /// appeared in an ingested event. An unknown key is `UnknownKey`, not a
/// silently dropped member. If your caller cannot guarantee that, pre-filter /// silently dropped member. If your caller cannot guarantee that, pre-filter
/// with [`History::lookup`] or [`History::current_skill`]; treating the /// with [`History::current_skill`] or [`History::competitors`]; treating the
/// error as "no information" and substituting a neutral value turns a /// error as "no information" and substituting a neutral value turns a
/// whole-team miss into a plausible constant, which is invisible to any /// whole-team miss into a plausible constant, which is invisible to any
/// test that does not assert on variation. /// test that does not assert on variation.
@@ -1966,7 +1953,7 @@ impl<T: Time, D: Drift<T>, O: Observer<T>, K: Eq + Hash + Clone> History<T, D, O
/// Every key must already be known to the history — that is, must have /// Every key must already be known to the history — that is, must have
/// appeared in an ingested event. An unknown key is `UnknownKey`, not a /// appeared in an ingested event. An unknown key is `UnknownKey`, not a
/// silently dropped member. If your caller cannot guarantee that, pre-filter /// silently dropped member. If your caller cannot guarantee that, pre-filter
/// with [`History::lookup`] or [`History::current_skill`]; treating the /// with [`History::current_skill`] or [`History::competitors`]; treating the
/// error as "no information" and substituting a neutral value turns a /// error as "no information" and substituting a neutral value turns a
/// whole-team miss into a plausible constant, which is invisible to any /// whole-team miss into a plausible constant, which is invisible to any
/// test that does not assert on variation. /// test that does not assert on variation.
@@ -2016,7 +2003,7 @@ impl<T: Time, D: Drift<T>, O: Observer<T>, K: Eq + Hash + Clone> History<T, D, O
/// Every key must already be known to the history — that is, must have /// Every key must already be known to the history — that is, must have
/// appeared in an ingested event. An unknown key is `UnknownKey`, not a /// appeared in an ingested event. An unknown key is `UnknownKey`, not a
/// silently dropped member. If your caller cannot guarantee that, pre-filter /// silently dropped member. If your caller cannot guarantee that, pre-filter
/// with [`History::lookup`] or [`History::current_skill`]; treating the /// with [`History::current_skill`] or [`History::competitors`]; treating the
/// error as "no information" and substituting a neutral value turns a /// error as "no information" and substituting a neutral value turns a
/// whole-team miss into a plausible constant, which is invisible to any /// whole-team miss into a plausible constant, which is invisible to any
/// test that does not assert on variation. /// test that does not assert on variation.
+11 -12
View File
@@ -290,23 +290,22 @@ pub(crate) const N_INF: Gaussian = Gaussian::from_ms(0.0, f64::INFINITY);
/// An interned competitor handle: a dense slot number, not a user key. /// An interned competitor handle: a dense slot number, not a user key.
/// ///
/// [`History`] stores skills and messages by `Index` rather than by `K`, so /// `History` stores skills and messages by `Index` rather than by `K`, so the
/// the hot path never hashes a key. [`History::intern`] promotes a key to one /// hot path never hashes a key. Indices are assigned in interning order and
/// and [`History::lookup`] resolves an existing key without creating. /// are stable for the life of a history; they are not portable between
/// histories, since the same key interns to a different slot under a different
/// ingestion order.
/// ///
/// Indices are assigned in interning order and are stable for the life of a /// Crate-internal. It was public, along with `History::intern` and
/// history. They are **not** portable between histories: the same key interns /// `History::lookup` that produced one — and **nothing public ever accepted
/// to different slots depending on ingestion order. /// one**, so it was a handle with nowhere to go. It also shadowed
/// `std::ops::Index`, which `CompetitorStore` implements. See #73.
#[derive(Copy, Clone, Default, PartialEq, PartialOrd, Eq, Ord, Hash, Debug)] #[derive(Copy, Clone, Default, PartialEq, PartialOrd, Eq, Ord, Hash, Debug)]
pub struct Index(usize); pub(crate) struct Index(usize);
impl Index { impl Index {
/// The underlying slot number. /// The underlying slot number.
/// pub(crate) fn get(self) -> usize {
/// Indices are dense and assigned in interning order, so this is usable as
/// a key into a caller-side side table.
#[must_use]
pub fn get(self) -> usize {
self.0 self.0
} }
} }
+6 -6
View File
@@ -41,9 +41,9 @@ fn add_events_bulk_via_iter() {
h.add_events(events).unwrap(); h.add_events(events).unwrap();
let report = h.converge().unwrap(); let report = h.converge().unwrap();
assert!(report.converged); assert!(report.converged);
assert!(h.lookup(&"a").is_some()); assert!(h.current_skill("a").is_some());
assert!(h.lookup(&"b").is_some()); assert!(h.current_skill("b").is_some());
assert!(h.lookup(&"c").is_some()); assert!(h.current_skill("c").is_some());
} }
#[test] #[test]
@@ -103,9 +103,9 @@ fn fluent_event_builder_basic() {
let report = h.converge().unwrap(); let report = h.converge().unwrap();
assert!(report.converged); assert!(report.converged);
assert!(h.lookup(&"alice").is_some()); assert!(h.current_skill("alice").is_some());
assert!(h.lookup(&"bob").is_some()); assert!(h.current_skill("bob").is_some());
assert!(h.lookup(&"carol").is_some()); assert!(h.current_skill("carol").is_some());
} }
#[test] #[test]
+8 -7
View File
@@ -77,17 +77,18 @@ fn linear_combinations_take_bare_keys() {
); );
} }
/// `lookup` is gone with `Index` (#73); the accessors that answer the same
/// question all take a borrowed key.
#[test] #[test]
fn lookup_accepts_a_borrowed_key_like_its_neighbours() { fn membership_queries_accept_a_borrowed_key() {
// `lookup` carried `ToOwned<Owned = K>`, copy-pasted from `intern`, which
// genuinely needs it to create the entry. `lookup` never creates.
let h = owned(); let h = owned();
assert!(h.lookup("alice").is_some());
assert!(h.lookup("nobody").is_none());
// Control: its neighbours already accepted this and must still.
assert!(h.current_skill("alice").is_some()); assert!(h.current_skill("alice").is_some());
assert!(h.rating("alice").is_some()); assert!(h.rating("alice").is_some());
assert!(h.learning_curve("alice").is_some());
assert!(h.current_skill("nobody").is_none());
assert!(h.rating("nobody").is_none());
assert!(h.learning_curve("nobody").is_none());
} }
#[test] #[test]
+20 -12
View File
@@ -17,24 +17,32 @@ fn record_winner_builds_history() {
h.record_winner(&"alice", &"bob", 1).unwrap(); h.record_winner(&"alice", &"bob", 1).unwrap();
let _ = h.converge().unwrap(); let _ = h.converge().unwrap();
let a_idx = h.lookup(&"alice").unwrap(); // `lookup` returned an `Index` that nothing public accepted, so the
let b_idx = h.lookup(&"bob").unwrap(); // observable claim is the one worth making: two distinct competitors, each
// with their own posterior, and the winner ahead.
assert_ne!(a_idx, b_idx); assert_eq!(h.competitor_count(), 2);
let alice = h.current_skill("alice").expect("alice played");
let bob = h.current_skill("bob").expect("bob played");
assert!(alice.mu() > bob.mu());
} }
/// The same key names the same competitor across events, which is what
/// interning bought and the only part of it a caller can observe.
#[test] #[test]
fn intern_is_idempotent() { fn a_repeated_key_is_one_competitor() {
let mut h: History = History::builder().build(); let mut h: History = History::builder().build();
let a1 = h.intern(&"alice"); h.record_winner(&"alice", &"bob", 1).unwrap();
let a2 = h.intern(&"alice"); h.record_winner(&"alice", &"carol", 2).unwrap();
assert_eq!(a1, a2);
assert_eq!(h.competitor_count(), 3);
assert_eq!(h.learning_curve("alice").expect("known").len(), 2);
} }
#[test] #[test]
fn lookup_returns_none_for_missing() { fn an_unknown_key_is_unknown() {
let h: History = History::builder().build(); let h: History = History::builder().build();
assert!(h.lookup(&"nobody").is_none()); assert!(h.current_skill("nobody").is_none());
assert!(h.learning_curve("nobody").is_none());
} }
#[test] #[test]
@@ -50,6 +58,6 @@ fn record_draw_with_p_draw_set() {
h.record_draw(&"alice", &"bob", 1).unwrap(); h.record_draw(&"alice", &"bob", 1).unwrap();
let _ = h.converge().unwrap(); let _ = h.converge().unwrap();
assert!(h.lookup(&"alice").is_some()); assert!(h.current_skill("alice").is_some());
assert!(h.lookup(&"bob").is_some()); assert!(h.current_skill("bob").is_some());
} }