From e4a68ba1a70289dbaf5829fece5a540f92ed9f01 Mon Sep 17 00:00:00 2001 From: Anders Olsson Date: Wed, 9 Sep 2026 21:13:35 +0200 Subject: [PATCH] fix!: per-key queries report unknown keys instead of a plausible constant MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) Claude-Session: https://claude.ai/code/session_011hcFjNDmHXZF8URGLku5zZ --- examples/atp.rs | 4 +- src/history.rs | 93 +++++++++++++++++---------- tests/api_shape.rs | 4 +- tests/degenerate_inputs.rs | 11 ++-- tests/event_builder_members.rs | 2 +- tests/filtered.rs | 14 ++--- tests/honest_accessors.rs | 112 +++++++++++++++++++++++++++++++++ tests/properties.rs | 7 ++- tests/registration.rs | 2 +- tests/time_axis.rs | 4 +- 10 files changed, 201 insertions(+), 52 deletions(-) create mode 100644 tests/honest_accessors.rs diff --git a/examples/atp.rs b/examples/atp.rs index df9cb93..fbd34d4 100644 --- a/examples/atp.rs +++ b/examples/atp.rs @@ -97,7 +97,7 @@ fn main() { let mut y_spec = (f64::MAX, f64::MIN); 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 { continue; } @@ -143,7 +143,7 @@ fn main() { let mut upper = 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 { continue; } diff --git a/src/history.rs b/src/history.rs index cd2e2bc..6f4cafe 100644 --- a/src/history.rs +++ b/src/history.rs @@ -745,19 +745,24 @@ impl, O: Observer, K: Eq + Hash + Clone> History(&self, key: &Q) -> Vec<(T, Gaussian)> + pub fn learning_curve(&self, key: &Q) -> Option> where K: std::borrow::Borrow, Q: std::hash::Hash + Eq + ?Sized, { - let Some(idx) = self.keys.get(key) else { - return Vec::new(); - }; - self.time_slices - .iter() - .filter_map(|ts| ts.skills.get(idx).map(|sk| (ts.time, sk.posterior()))) - .collect() + let idx = self.keys.get(key)?; + Some( + self.time_slices + .iter() + .filter_map(|ts| ts.skills.get(idx).map(|sk| (ts.time, sk.posterior()))) + .collect(), + ) } /// Filtered learning curves for all competitors, keyed by user-facing key. @@ -792,25 +797,27 @@ impl, O: Observer, K: Eq + Hash + Clone> History(&self, key: &Q) -> Vec<(T, Gaussian)> + pub fn filtered_learning_curve(&self, key: &Q) -> Option> where K: Borrow, Q: Hash + Eq + ?Sized, { - let Some(idx) = self.keys.get(key) else { - return Vec::new(); - }; + let idx = self.keys.get(key)?; - self.filtered_pass() - .into_iter() - .filter_map(|(time, step)| { - step.posteriors - .iter() - .find(|(competitor, _)| *competitor == idx) - .map(|&(_, posterior)| (time, posterior)) - }) - .collect() + Some( + self.filtered_pass() + .into_iter() + .filter_map(|(time, step)| { + step.posteriors + .iter() + .find(|(competitor, _)| *competitor == idx) + .map(|&(_, posterior)| (time, posterior)) + }) + .collect(), + ) } /// Sum per-slice evidence. @@ -855,14 +862,36 @@ impl, O: Observer, K: Eq + Hash + Clone> History(&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(&self, keys: &[&Q]) -> Result where K: std::borrow::Borrow, - Q: std::hash::Hash + Eq + ?Sized, + Q: std::hash::Hash + Eq + ?Sized + std::fmt::Debug, { - let targets: Vec = keys.iter().filter_map(|k| self.keys.get(*k)).collect(); - self.log_evidence_internal(false, &targets) + let mut targets: Vec = Vec::with_capacity(keys.len()); + 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. @@ -2961,8 +2990,8 @@ mod tests { h.add_events(events).unwrap(); let _ = h.converge().unwrap(); - let lc_a = h.learning_curve("a"); - let lc_c = h.learning_curve("c"); + let lc_a = h.learning_curve("a").unwrap(); + let lc_c = h.learning_curve("c").unwrap(); let aj_e = lc_a.len(); let cj_e = lc_c.len(); @@ -3640,8 +3669,8 @@ mod tests { ]; h.add_events(events).unwrap(); - let lc_a = h.learning_curve("a"); - let lc_b = h.learning_curve("b"); + let lc_a = h.learning_curve("a").unwrap(); + let lc_b = h.learning_curve("b").unwrap(); assert_ulps_eq!( lc_a[0].1, @@ -3666,8 +3695,8 @@ mod tests { let _ = h.converge().unwrap(); - let lc_a = h.learning_curve("a"); - let lc_b = h.learning_curve("b"); + let lc_a = h.learning_curve("a").unwrap(); + 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_b[0].1, lc_a[0].1, epsilon = 1e-6); diff --git a/tests/api_shape.rs b/tests/api_shape.rs index 31656d5..6466b2f 100644 --- a/tests/api_shape.rs +++ b/tests/api_shape.rs @@ -162,7 +162,7 @@ fn current_skill_and_learning_curve() { let b = h.current_skill(&"b").unwrap(); 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[0].0, 1); 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(&"b", &"a", 2).unwrap(); 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!(a_only.is_finite()); } diff --git a/tests/degenerate_inputs.rs b/tests/degenerate_inputs.rs index 0510630..ec1acaf 100644 --- a/tests/degenerate_inputs.rs +++ b/tests/degenerate_inputs.rs @@ -184,7 +184,10 @@ fn event_builder_weights_mismatch_leaves_the_history_untouched() { .winner(0) .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] @@ -199,7 +202,7 @@ fn empty_event_stream_then_converge() { fn empty_history_queries_do_not_panic() { let h = History::default(); 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()); } @@ -322,7 +325,7 @@ fn empty_history_has_no_filtered_estimates() { 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) ---------------------------------------------- @@ -337,7 +340,7 @@ fn tight() -> ConvergenceOptions { fn assert_curve_finite(h: &History, keys: &[&str], what: &str) { for key in keys { - for (time, g) in h.learning_curve(*key) { + for (time, g) in h.learning_curve(*key).unwrap() { assert!( g.mu().is_finite() && g.sigma().is_finite(), "{what}: non-finite posterior for {key} at t={time} (mu={} sigma={})", diff --git a/tests/event_builder_members.rs b/tests/event_builder_members.rs index cd098eb..96f883d 100644 --- a/tests/event_builder_members.rs +++ b/tests/event_builder_members.rs @@ -81,7 +81,7 @@ fn members_matches_the_typed_path_exactly() { #[test] fn a_drift_scale_set_through_members_is_applied() { 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"); let (lo, hi) = curve.iter().fold((f64::MAX, f64::MIN), |(lo, hi), (_, g)| { (lo.min(g.sigma()), hi.max(g.sigma())) diff --git a/tests/filtered.rs b/tests/filtered.rs index 7265f09..f10ed69 100644 --- a/tests/filtered.rs +++ b/tests/filtered.rs @@ -73,8 +73,8 @@ fn filtered_first_point_is_less_certain_than_smoothed() { let _ = history.converge().unwrap(); - let smoothed = history.learning_curve("a"); - let filtered = history.filtered_learning_curve("a"); + let smoothed = history.learning_curve("a").unwrap(); + let filtered = history.filtered_learning_curve("a").unwrap(); assert_eq!( smoothed.len(), @@ -127,7 +127,7 @@ fn filtered_curves_plural_agrees_with_singular() { assert_eq!( 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" ); } @@ -182,8 +182,8 @@ fn single_slice_filtered_matches_smoothed() { let _ = history.converge().unwrap(); - let smoothed = history.learning_curve("a"); - let filtered = history.filtered_learning_curve("a"); + let smoothed = history.learning_curve("a").unwrap(); + let filtered = history.filtered_learning_curve("a").unwrap(); assert_eq!(smoothed.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 from_batched = batched.filtered_learning_curve("a"); - let from_incremental = incremental.filtered_learning_curve("a"); + let from_batched = batched.filtered_learning_curve("a").unwrap(); + let from_incremental = incremental.filtered_learning_curve("a").unwrap(); assert_eq!(from_batched.len(), from_incremental.len()); diff --git a/tests/honest_accessors.rs b/tests/honest_accessors.rs new file mode 100644 index 0000000..22a9dc6 --- /dev/null +++ b/tests/honest_accessors.rs @@ -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; + +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"); +} diff --git a/tests/properties.rs b/tests/properties.rs index 9f04310..9e17592 100644 --- a/tests/properties.rs +++ b/tests/properties.rs @@ -67,7 +67,12 @@ proptest! { let _ = h.converge().unwrap(); 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}")); } } diff --git a/tests/registration.rs b/tests/registration.rs index 45555ae..507b7d5 100644 --- a/tests/registration.rs +++ b/tests/registration.rs @@ -119,7 +119,7 @@ fn registration_reaches_a_competitor_first_seen_through_record_winner() { assert_eq!(rating.prior().mu(), PINNED.mu()); // 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); let widest = curve .iter() diff --git a/tests/time_axis.rs b/tests/time_axis.rs index 7c38438..894157c 100644 --- a/tests/time_axis.rs +++ b/tests/time_axis.rs @@ -94,7 +94,7 @@ fn a_custom_time_type_and_a_custom_drift_work_together() { } 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:?}"); for (season, g) in &curve { assert!( @@ -148,5 +148,5 @@ fn new_constructs_on_any_axis_directly() { h.record_winner(&"a".to_string(), &"b".to_string(), Season(7)) .unwrap(); 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)); }