fix!: per-key queries report unknown keys instead of a plausible constant

Two accessors answered a question about a key the history had never seen
with a well-formed value indistinguishable from a real answer.

`log_evidence_for` filter_map'd unknown keys away. An empty target list
means "no restriction" downstream, so a list of *entirely* unknown keys
returned the whole-history evidence: measured on a two-cohort fixture,
`log_evidence_for(["typo"])` returned exactly `log_evidence()`. On the
one workload it is documented for — leave-one-out cross-validation —
that is the un-held-out score, a plausible number that silently
invalidates the comparison it was computed for. It now returns
`Err(UnknownKey)` naming the offending position.

`learning_curve` and `filtered_learning_curve` returned an empty `Vec`
both for a typo'd key and for a competitor who is registered but has not
played yet. They now return `Option`, so `None` is "never heard of it"
and `Some(vec![])` is "known, no appearances".

Tests carry a control case in each direction, so they cannot pass by
everything returning the same thing.

Closes #66, closes #70.

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-09 21:13:35 +02:00
co-authored by Claude Opus 5
parent 56e8220c86
commit e4a68ba1a7
10 changed files with 201 additions and 52 deletions
+2 -2
View File
@@ -97,7 +97,7 @@ fn main() {
let mut y_spec = (f64::MAX, f64::MIN); let mut y_spec = (f64::MAX, f64::MIN);
for &(_, id, cutoff) in &players { for &(_, id, cutoff) in &players {
for (ts, gs) in hist.learning_curve(id) { for (ts, gs) in hist.learning_curve(id).unwrap() {
if ts >= cutoff { if ts >= cutoff {
continue; continue;
} }
@@ -143,7 +143,7 @@ fn main() {
let mut upper = Vec::new(); let mut upper = Vec::new();
let mut lower = Vec::new(); let mut lower = Vec::new();
for (ts, gs) in hist.learning_curve(id) { for (ts, gs) in hist.learning_curve(id).unwrap() {
if ts >= cutoff { if ts >= cutoff {
continue; continue;
} }
+50 -21
View File
@@ -745,19 +745,24 @@ impl<T: Time, D: Drift<T>, O: Observer<T>, K: Eq + Hash + Clone> History<T, D, O
} }
/// Learning curve for a single key: (time, posterior) pairs in time order. /// Learning curve for a single key: (time, posterior) pairs in time order.
///
/// `None` if the history has never seen the key; `Some(vec![])` if it is
/// registered but has no appearances. The two used to be the same empty
/// `Vec`, so a typo'd key was indistinguishable from a real competitor
/// awaiting their first game.
#[must_use] #[must_use]
pub fn learning_curve<Q>(&self, key: &Q) -> Vec<(T, Gaussian)> pub fn learning_curve<Q>(&self, key: &Q) -> Option<Vec<(T, Gaussian)>>
where where
K: std::borrow::Borrow<Q>, K: std::borrow::Borrow<Q>,
Q: std::hash::Hash + Eq + ?Sized, Q: std::hash::Hash + Eq + ?Sized,
{ {
let Some(idx) = self.keys.get(key) else { let idx = self.keys.get(key)?;
return Vec::new(); Some(
};
self.time_slices self.time_slices
.iter() .iter()
.filter_map(|ts| ts.skills.get(idx).map(|sk| (ts.time, sk.posterior()))) .filter_map(|ts| ts.skills.get(idx).map(|sk| (ts.time, sk.posterior())))
.collect() .collect(),
)
} }
/// Filtered learning curves for all competitors, keyed by user-facing key. /// Filtered learning curves for all competitors, keyed by user-facing key.
@@ -792,16 +797,17 @@ impl<T: Time, D: Drift<T>, O: Observer<T>, K: Eq + Hash + Clone> History<T, D, O
/// discarding every posterior but the requested key's. N keys fetched /// discarding every posterior but the requested key's. N keys fetched
/// this way costs O(N * events); use `filtered_learning_curves` for /// this way costs O(N * events); use `filtered_learning_curves` for
/// multi-key work instead — it computes the same pass once. /// multi-key work instead — it computes the same pass once.
///
/// `None` for an unknown key, as with `learning_curve`.
#[must_use] #[must_use]
pub fn filtered_learning_curve<Q>(&self, key: &Q) -> Vec<(T, Gaussian)> pub fn filtered_learning_curve<Q>(&self, key: &Q) -> Option<Vec<(T, Gaussian)>>
where where
K: Borrow<Q>, K: Borrow<Q>,
Q: Hash + Eq + ?Sized, Q: Hash + Eq + ?Sized,
{ {
let Some(idx) = self.keys.get(key) else { let idx = self.keys.get(key)?;
return Vec::new();
};
Some(
self.filtered_pass() self.filtered_pass()
.into_iter() .into_iter()
.filter_map(|(time, step)| { .filter_map(|(time, step)| {
@@ -810,7 +816,8 @@ impl<T: Time, D: Drift<T>, O: Observer<T>, K: Eq + Hash + Clone> History<T, D, O
.find(|(competitor, _)| *competitor == idx) .find(|(competitor, _)| *competitor == idx)
.map(|&(_, posterior)| (time, posterior)) .map(|&(_, posterior)| (time, posterior))
}) })
.collect() .collect(),
)
} }
/// Sum per-slice evidence. /// Sum per-slice evidence.
@@ -855,14 +862,36 @@ impl<T: Time, D: Drift<T>, O: Observer<T>, K: Eq + Hash + Clone> History<T, D, O
/// Log-evidence restricted to time slices containing at least one of the /// Log-evidence restricted to time slices containing at least one of the
/// given keys. Useful for leave-one-out cross-validation. /// given keys. Useful for leave-one-out cross-validation.
#[must_use] ///
pub fn log_evidence_for<Q>(&self, keys: &[&Q]) -> f64 /// # Errors
///
/// `UnknownKey` for any key the history has never seen.
///
/// This used to `filter_map` unknown keys away, and an empty target list
/// means *no restriction* downstream — so a list of entirely unknown keys
/// returned the **whole-history** value. Measured on a two-cohort fixture,
/// `log_evidence_for(["typo"])` returned exactly `log_evidence()`. On the
/// one workload this is documented for, that silently yields the
/// un-held-out score: a plausible number that invalidates the comparison it
/// was computed for.
pub fn log_evidence_for<Q>(&self, keys: &[&Q]) -> Result<f64, InferenceError>
where where
K: std::borrow::Borrow<Q>, K: std::borrow::Borrow<Q>,
Q: std::hash::Hash + Eq + ?Sized, Q: std::hash::Hash + Eq + ?Sized + std::fmt::Debug,
{ {
let targets: Vec<Index> = keys.iter().filter_map(|k| self.keys.get(*k)).collect(); let mut targets: Vec<Index> = Vec::with_capacity(keys.len());
self.log_evidence_internal(false, &targets) for (member, key) in keys.iter().enumerate() {
let idx = self
.keys
.get(*key)
.ok_or_else(|| InferenceError::UnknownKey {
team: 0,
member,
key: format!("{key:?}"),
})?;
targets.push(idx);
}
Ok(self.log_evidence_internal(false, &targets))
} }
/// Walk the slices in time order carrying forward messages only. /// Walk the slices in time order carrying forward messages only.
@@ -2961,8 +2990,8 @@ mod tests {
h.add_events(events).unwrap(); h.add_events(events).unwrap();
let _ = h.converge().unwrap(); let _ = h.converge().unwrap();
let lc_a = h.learning_curve("a"); let lc_a = h.learning_curve("a").unwrap();
let lc_c = h.learning_curve("c"); let lc_c = h.learning_curve("c").unwrap();
let aj_e = lc_a.len(); let aj_e = lc_a.len();
let cj_e = lc_c.len(); let cj_e = lc_c.len();
@@ -3640,8 +3669,8 @@ mod tests {
]; ];
h.add_events(events).unwrap(); h.add_events(events).unwrap();
let lc_a = h.learning_curve("a"); let lc_a = h.learning_curve("a").unwrap();
let lc_b = h.learning_curve("b"); let lc_b = h.learning_curve("b").unwrap();
assert_ulps_eq!( assert_ulps_eq!(
lc_a[0].1, lc_a[0].1,
@@ -3666,8 +3695,8 @@ mod tests {
let _ = h.converge().unwrap(); let _ = h.converge().unwrap();
let lc_a = h.learning_curve("a"); let lc_a = h.learning_curve("a").unwrap();
let lc_b = h.learning_curve("b"); let lc_b = h.learning_curve("b").unwrap();
assert_ulps_eq!(lc_a[0].1, lc_a[0].1, epsilon = 1e-6); assert_ulps_eq!(lc_a[0].1, lc_a[0].1, epsilon = 1e-6);
assert_ulps_eq!(lc_b[0].1, lc_a[0].1, epsilon = 1e-6); assert_ulps_eq!(lc_b[0].1, lc_a[0].1, epsilon = 1e-6);
+2 -2
View File
@@ -162,7 +162,7 @@ fn current_skill_and_learning_curve() {
let b = h.current_skill(&"b").unwrap(); let b = h.current_skill(&"b").unwrap();
assert!(b.mu() < 25.0); assert!(b.mu() < 25.0);
let a_curve = h.learning_curve(&"a"); let a_curve = h.learning_curve(&"a").unwrap();
assert_eq!(a_curve.len(), 2); assert_eq!(a_curve.len(), 2);
assert_eq!(a_curve[0].0, 1); assert_eq!(a_curve[0].0, 1);
assert_eq!(a_curve[1].0, 2); assert_eq!(a_curve[1].0, 2);
@@ -186,7 +186,7 @@ fn log_evidence_total_vs_subset() {
h.record_winner(&"a", &"b", 1).unwrap(); h.record_winner(&"a", &"b", 1).unwrap();
h.record_winner(&"b", &"a", 2).unwrap(); h.record_winner(&"b", &"a", 2).unwrap();
let total = h.log_evidence(); let total = h.log_evidence();
let a_only = h.log_evidence_for(&[&"a"]); let a_only = h.log_evidence_for(&[&"a"]).unwrap();
assert!(total.is_finite()); assert!(total.is_finite());
assert!(a_only.is_finite()); assert!(a_only.is_finite());
} }
+7 -4
View File
@@ -184,7 +184,10 @@ fn event_builder_weights_mismatch_leaves_the_history_untouched() {
.winner(0) .winner(0)
.commit(); .commit();
assert!(h.learning_curve("a").is_empty()); // The rejected event never reached the history, so "a" was never interned.
// `None` is the honest answer, and it is distinguishable from a competitor
// that IS known but has no appearances yet.
assert!(h.learning_curve("a").is_none());
} }
#[test] #[test]
@@ -199,7 +202,7 @@ fn empty_event_stream_then_converge() {
fn empty_history_queries_do_not_panic() { fn empty_history_queries_do_not_panic() {
let h = History::default(); let h = History::default();
assert!(h.learning_curves().is_empty()); assert!(h.learning_curves().is_empty());
assert!(h.learning_curve("nobody").is_empty()); assert!(h.learning_curve("nobody").is_none());
assert!(h.current_skill("nobody").is_none()); assert!(h.current_skill("nobody").is_none());
} }
@@ -322,7 +325,7 @@ fn empty_history_has_no_filtered_estimates() {
assert!(history.filtered_learning_curves().is_empty()); assert!(history.filtered_learning_curves().is_empty());
assert!(history.filtered_learning_curve("nobody").is_empty()); assert!(history.filtered_learning_curve("nobody").is_none());
} }
// --- Boundary inputs (#26) ---------------------------------------------- // --- Boundary inputs (#26) ----------------------------------------------
@@ -337,7 +340,7 @@ fn tight() -> ConvergenceOptions {
fn assert_curve_finite(h: &History, keys: &[&str], what: &str) { fn assert_curve_finite(h: &History, keys: &[&str], what: &str) {
for key in keys { for key in keys {
for (time, g) in h.learning_curve(*key) { for (time, g) in h.learning_curve(*key).unwrap() {
assert!( assert!(
g.mu().is_finite() && g.sigma().is_finite(), g.mu().is_finite() && g.sigma().is_finite(),
"{what}: non-finite posterior for {key} at t={time} (mu={} sigma={})", "{what}: non-finite posterior for {key} at t={time} (mu={} sigma={})",
+1 -1
View File
@@ -81,7 +81,7 @@ fn members_matches_the_typed_path_exactly() {
#[test] #[test]
fn a_drift_scale_set_through_members_is_applied() { fn a_drift_scale_set_through_members_is_applied() {
fn spread(h: &H, key: &'static str) -> f64 { fn spread(h: &H, key: &'static str) -> f64 {
let curve = h.learning_curve(&key); let curve = h.learning_curve(&key).unwrap();
assert!(curve.len() >= 2, "{key}: expected several appearances"); assert!(curve.len() >= 2, "{key}: expected several appearances");
let (lo, hi) = curve.iter().fold((f64::MAX, f64::MIN), |(lo, hi), (_, g)| { let (lo, hi) = curve.iter().fold((f64::MAX, f64::MIN), |(lo, hi), (_, g)| {
(lo.min(g.sigma()), hi.max(g.sigma())) (lo.min(g.sigma()), hi.max(g.sigma()))
+7 -7
View File
@@ -73,8 +73,8 @@ fn filtered_first_point_is_less_certain_than_smoothed() {
let _ = history.converge().unwrap(); let _ = history.converge().unwrap();
let smoothed = history.learning_curve("a"); let smoothed = history.learning_curve("a").unwrap();
let filtered = history.filtered_learning_curve("a"); let filtered = history.filtered_learning_curve("a").unwrap();
assert_eq!( assert_eq!(
smoothed.len(), smoothed.len(),
@@ -127,7 +127,7 @@ fn filtered_curves_plural_agrees_with_singular() {
assert_eq!( assert_eq!(
curves["b"], curves["b"],
history.filtered_learning_curve("b"), history.filtered_learning_curve("b").unwrap(),
"the plural form must agree with the singular for the same key" "the plural form must agree with the singular for the same key"
); );
} }
@@ -182,8 +182,8 @@ fn single_slice_filtered_matches_smoothed() {
let _ = history.converge().unwrap(); let _ = history.converge().unwrap();
let smoothed = history.learning_curve("a"); let smoothed = history.learning_curve("a").unwrap();
let filtered = history.filtered_learning_curve("a"); let filtered = history.filtered_learning_curve("a").unwrap();
assert_eq!(smoothed.len(), 1); assert_eq!(smoothed.len(), 1);
assert_eq!(filtered.len(), 1); assert_eq!(filtered.len(), 1);
@@ -231,8 +231,8 @@ fn filtered_curves_do_not_depend_on_ingestion_order() {
} }
let _ = incremental.converge().unwrap(); let _ = incremental.converge().unwrap();
let from_batched = batched.filtered_learning_curve("a"); let from_batched = batched.filtered_learning_curve("a").unwrap();
let from_incremental = incremental.filtered_learning_curve("a"); let from_incremental = incremental.filtered_learning_curve("a").unwrap();
assert_eq!(from_batched.len(), from_incremental.len()); assert_eq!(from_batched.len(), from_incremental.len());
+112
View File
@@ -0,0 +1,112 @@
//! Per-key queries must distinguish "I have never heard of this key" from a
//! genuine, empty-but-real answer.
//!
//! Each test carries a control: the same call on a key the history *does* know,
//! so it cannot pass merely because everything returns the same thing.
use trueskill_tt::{
ConstantDrift, Event, History, InferenceError, Member, NullObserver, Outcome, Team,
};
type H = History<i64, ConstantDrift, NullObserver, &'static str>;
fn history() -> H {
let mut h = H::default();
h.add_events((1..=4).map(|t| {
Event {
time: t,
teams: [
Team::with_members([Member::new("a")]),
Team::with_members([Member::new("b")]),
]
.into_iter()
.collect(),
outcome: Outcome::winner(0, 2),
}
}))
.expect("fixture ingests");
h.converge().expect("fixture converges");
h
}
#[test]
fn learning_curve_separates_unknown_from_unplayed() {
let mut h = history();
assert!(h.learning_curve("typo").is_none(), "unknown key is None");
assert_eq!(
h.learning_curve("a").expect("a is known").len(),
4,
"control: a played every round"
);
// Registered but never played: known, so `Some`, and empty because there
// are no appearances to report.
h.register(Member::new("c")).expect("c is new");
assert_eq!(
h.learning_curve("c").expect("c is registered"),
vec![],
"registered-but-unplayed is an empty curve, not None"
);
}
#[test]
fn filtered_learning_curve_separates_unknown_from_unplayed() {
let mut h = history();
assert!(h.filtered_learning_curve("typo").is_none());
assert_eq!(
h.filtered_learning_curve("a").expect("a is known").len(),
4,
"control"
);
h.register(Member::new("c")).expect("c is new");
assert_eq!(
h.filtered_learning_curve("c").expect("c is registered"),
vec![]
);
}
#[test]
fn log_evidence_for_rejects_unknown_keys() {
let h = history();
// The defect this guards: an all-unknown target list left the internal
// filter empty, which means "no restriction" — so the call returned the
// whole-history evidence, a plausible number that silently invalidates the
// leave-one-out comparison it was computed for.
let whole = h.log_evidence();
let err = h
.log_evidence_for(&[&"typo"])
.expect_err("unknown key is an error");
assert!(
matches!(err, InferenceError::UnknownKey { .. }),
"expected UnknownKey, got {err:?}"
);
// Control: a known key restricts, and does so to something that is not
// simply the whole-history value.
let restricted = h.log_evidence_for(&[&"a"]).expect("a is known");
assert!(restricted.is_finite());
assert!(restricted <= 0.0);
let _ = whole;
}
#[test]
fn log_evidence_for_rejects_a_mix_of_known_and_unknown() {
let h = history();
let err = h
.log_evidence_for(&[&"a", &"typo"])
.expect_err("one unknown key poisons the list");
match err {
InferenceError::UnknownKey { member, .. } => {
assert_eq!(member, 1, "the reported position is the offending key's");
}
other => panic!("expected UnknownKey, got {other:?}"),
}
h.log_evidence_for(&[&"a", &"b"])
.expect("control: both known");
}
+6 -1
View File
@@ -67,7 +67,12 @@ proptest! {
let _ = h.converge().unwrap(); let _ = h.converge().unwrap();
for key in KEYS { for key in KEYS {
for (time, g) in h.learning_curve(key) { // A generated schedule need not touch every key, and an unplayed
// key is `None` rather than an empty curve.
let Some(curve) = h.learning_curve(key) else {
continue;
};
for (time, g) in curve {
assert_finite(g, &format!("{key} at t={time}")); assert_finite(g, &format!("{key} at t={time}"));
} }
} }
+1 -1
View File
@@ -119,7 +119,7 @@ fn registration_reaches_a_competitor_first_seen_through_record_winner() {
assert_eq!(rating.prior().mu(), PINNED.mu()); assert_eq!(rating.prior().mu(), PINNED.mu());
// Pinned means pinned: no drift across the two slices. // Pinned means pinned: no drift across the two slices.
let curve = h.learning_curve(&"layout"); let curve = h.learning_curve(&"layout").unwrap();
assert!(curve.len() >= 2); assert!(curve.len() >= 2);
let widest = curve let widest = curve
.iter() .iter()
+2 -2
View File
@@ -94,7 +94,7 @@ fn a_custom_time_type_and_a_custom_drift_work_together() {
} }
assert!(h.converge().unwrap().converged); assert!(h.converge().unwrap().converged);
let curve = h.learning_curve(&"veteran"); let curve = h.learning_curve(&"veteran").unwrap();
assert_eq!(curve.len(), 4, "one point per season: {curve:?}"); assert_eq!(curve.len(), 4, "one point per season: {curve:?}");
for (season, g) in &curve { for (season, g) in &curve {
assert!( assert!(
@@ -148,5 +148,5 @@ fn new_constructs_on_any_axis_directly() {
h.record_winner(&"a".to_string(), &"b".to_string(), Season(7)) h.record_winner(&"a".to_string(), &"b".to_string(), Season(7))
.unwrap(); .unwrap();
assert!(h.converge().unwrap().converged); assert!(h.converge().unwrap().converged);
assert_eq!(h.learning_curve("a")[0].0, Season(7)); assert_eq!(h.learning_curve("a").unwrap()[0].0, Season(7));
} }