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
This commit is contained in:
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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]
|
||||||
|
|||||||
@@ -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
@@ -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());
|
||||||
}
|
}
|
||||||
|
|||||||
Reference in New Issue
Block a user