fix: close out four small issues and pin #27's repro
#29 — log_evidence and log_evidence_for took &mut self while mutating nothing. Loosening them to &self is not source-breaking for ordinary callers (a &mut reborrows as & transparently) and brings them in line with the filtered_* accessors added last week. Not the mechanical change it looked like: under the rayon feature the closure in log_evidence_internal captured all of &self rather than just the competitor store, which drags KeyTable<K> in and demands K: Sync from every caller. That compiled while the method took &mut self and stopped compiling the moment it did not. Binding `let agents = &self.agents;` before the closure narrows the capture; the comment there says why, because the next person to inline it will reintroduce the bound. #31 — TimeSlice::add_events constructed Skill with ..Default::default() while filtered_step spells every field out. The design relies on a new Skill field being a compile error at construction sites rather than a silent default, and that tripwire only fired at one of the two. Now both. #28 — log_evidence_internal's `forward` flag is a genuine forward-only quantity only on a history that has never been converged, because iteration alternates sweeps and the likelihood feeding the forward message absorbs backward information from the second iteration onward. Documented, with a pointer to filtered_log_evidence for the quantity that survives convergence. That trap is one function away from the one #19 was about. #23 — color_greedy carried #[allow(dead_code)] despite being called by recompute_color_groups: a mute button on a live function, which is the specific complaint in that issue. #27 was already fixed — the guard landed inf4e2922and the issue was filed against7742b2b, which merge-base confirms predates it — but nothing pinned it. Added the issue's own reproduction, which matters because the two profiles fail differently and a debug-only test would miss the release path. Removing both guards reproduces the issue verbatim: "attempt to subtract with overflow" in debug, "index out of bounds: the len is 0 but the index is 18446744073709551615" in release. Also amended the filtered-estimates spec (#30): the tolerance-not-bit-identity caveat is conservative. Forcing the scratch onto the sequential sweep instead of the grouped one — a far larger perturbation than a permuted event order — still agrees within 1e-8 under tight convergence. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01T5SYDExxL4vZgvunrcNSMc
This commit is contained in:
@@ -312,6 +312,18 @@ inert.
|
|||||||
Results agree to within convergence tolerance rather than exactly.
|
Results agree to within convergence tolerance rather than exactly.
|
||||||
Normalising the order in the scratch builder would buy bit-identity at
|
Normalising the order in the scratch builder would buy bit-identity at
|
||||||
the cost of diverging from what the real sweep does; not worth it.
|
the cost of diverging from what the real sweep does; not worth it.
|
||||||
|
|
||||||
|
**Measured after implementation, this risk is smaller than stated.**
|
||||||
|
Flipping the scratch's `color_groups_dirty` from `true` to `false`
|
||||||
|
switches it between the grouped sweep (`sweep_color_groups`) and the
|
||||||
|
sequential fallback across its entire convergence loop — a far larger
|
||||||
|
perturbation than a permuted event order — and the ingestion-order
|
||||||
|
invariance test stays green at `1e-8` under `max_iter: 2_000`,
|
||||||
|
`epsilon: 1e-12`. EP reaches the same fixed point regardless of sweep
|
||||||
|
order once driven far enough. The tolerance caveat is correct but
|
||||||
|
conservative. Note the flag itself is load-bearing: with it `false` the
|
||||||
|
scratch would take the sequential path always, diverging from the
|
||||||
|
production sweep it exists to mirror.
|
||||||
- **Divergence risk.** If `TimeSlice`'s sweep gains state that the
|
- **Divergence risk.** If `TimeSlice`'s sweep gains state that the
|
||||||
scratch construction does not initialise, the pass silently reads a
|
scratch construction does not initialise, the pass silently reads a
|
||||||
default. The scratch builder must construct `Skill` field-by-field
|
default. The scratch builder must construct `Skill` field-by-field
|
||||||
|
|||||||
@@ -103,7 +103,6 @@ impl ColorGroups {
|
|||||||
/// `Index` values that event touches. The returned `ColorGroups` has one
|
/// `Index` values that event touches. The returned `ColorGroups` has one
|
||||||
/// inner `Vec<usize>` per color, containing event indices in the order
|
/// inner `Vec<usize>` per color, containing event indices in the order
|
||||||
/// they were assigned.
|
/// they were assigned.
|
||||||
#[allow(dead_code)]
|
|
||||||
pub(crate) fn color_greedy<I, F>(n_events: usize, index_set: F) -> ColorGroups
|
pub(crate) fn color_greedy<I, F>(n_events: usize, index_set: F) -> ColorGroups
|
||||||
where
|
where
|
||||||
F: Fn(usize) -> I,
|
F: Fn(usize) -> I,
|
||||||
|
|||||||
+19
-5
@@ -428,14 +428,28 @@ impl<T: Time, D: Drift<T>, O: Observer<T>, K: Eq + Hash + Clone> History<T, D, O
|
|||||||
.collect()
|
.collect()
|
||||||
}
|
}
|
||||||
|
|
||||||
pub(crate) fn log_evidence_internal(&mut self, forward: bool, targets: &[Index]) -> f64 {
|
/// Sum per-slice evidence.
|
||||||
|
///
|
||||||
|
/// `forward` selects `skill.forward` as each event's prior instead of the
|
||||||
|
/// cavity. That is a genuine forward-only (filtering) quantity ONLY on a
|
||||||
|
/// history that has never been converged: `iteration` alternates backward
|
||||||
|
/// and forward sweeps, so from the second iteration onward the likelihood
|
||||||
|
/// feeding the forward message has already absorbed backward information.
|
||||||
|
/// For a filtering quantity that holds after convergence, use
|
||||||
|
/// `filtered_log_evidence`.
|
||||||
|
pub(crate) fn log_evidence_internal(&self, forward: bool, targets: &[Index]) -> f64 {
|
||||||
|
// Bound before the closure so it captures the store rather than all of
|
||||||
|
// `&self`: capturing `&History` would drag `KeyTable<K>` in and demand
|
||||||
|
// `K: Sync` from every caller, which the key type need not satisfy.
|
||||||
|
let agents = &self.agents;
|
||||||
|
|
||||||
#[cfg(feature = "rayon")]
|
#[cfg(feature = "rayon")]
|
||||||
{
|
{
|
||||||
use rayon::prelude::*;
|
use rayon::prelude::*;
|
||||||
let per_slice: Vec<f64> = self
|
let per_slice: Vec<f64> = self
|
||||||
.time_slices
|
.time_slices
|
||||||
.par_iter()
|
.par_iter()
|
||||||
.map(|ts| ts.log_evidence(targets, forward, &self.agents))
|
.map(|ts| ts.log_evidence(targets, forward, agents))
|
||||||
.collect();
|
.collect();
|
||||||
per_slice.into_iter().sum()
|
per_slice.into_iter().sum()
|
||||||
}
|
}
|
||||||
@@ -443,19 +457,19 @@ impl<T: Time, D: Drift<T>, O: Observer<T>, K: Eq + Hash + Clone> History<T, D, O
|
|||||||
{
|
{
|
||||||
self.time_slices
|
self.time_slices
|
||||||
.iter()
|
.iter()
|
||||||
.map(|ts| ts.log_evidence(targets, forward, &self.agents))
|
.map(|ts| ts.log_evidence(targets, forward, agents))
|
||||||
.sum()
|
.sum()
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
/// Total log-evidence across the history.
|
/// Total log-evidence across the history.
|
||||||
pub fn log_evidence(&mut self) -> f64 {
|
pub fn log_evidence(&self) -> f64 {
|
||||||
self.log_evidence_internal(false, &[])
|
self.log_evidence_internal(false, &[])
|
||||||
}
|
}
|
||||||
|
|
||||||
/// 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.
|
||||||
pub fn log_evidence_for<Q>(&mut self, keys: &[&Q]) -> f64
|
pub fn log_evidence_for<Q>(&self, keys: &[&Q]) -> f64
|
||||||
where
|
where
|
||||||
K: std::borrow::Borrow<Q>,
|
K: std::borrow::Borrow<Q>,
|
||||||
Q: std::hash::Hash + Eq + ?Sized,
|
Q: std::hash::Hash + Eq + ?Sized,
|
||||||
|
|||||||
+2
-1
@@ -305,8 +305,9 @@ impl<T: Time> TimeSlice<T> {
|
|||||||
*idx,
|
*idx,
|
||||||
Skill {
|
Skill {
|
||||||
forward: agents[*idx].receive(&self.time),
|
forward: agents[*idx].receive(&self.time),
|
||||||
|
backward: N_INF,
|
||||||
|
likelihood: N_INF,
|
||||||
elapsed,
|
elapsed,
|
||||||
..Default::default()
|
|
||||||
},
|
},
|
||||||
);
|
);
|
||||||
}
|
}
|
||||||
|
|||||||
@@ -5,7 +5,7 @@
|
|||||||
|
|
||||||
use trueskill_tt::{
|
use trueskill_tt::{
|
||||||
ConstantDrift, ConvergenceOptions, Game, GameOptions, Gaussian, History, InferenceError,
|
ConstantDrift, ConvergenceOptions, Game, GameOptions, Gaussian, History, InferenceError,
|
||||||
Outcome, Rating,
|
NullObserver, Outcome, Rating,
|
||||||
};
|
};
|
||||||
|
|
||||||
type R = Rating<i64, ConstantDrift>;
|
type R = Rating<i64, ConstantDrift>;
|
||||||
@@ -127,6 +127,20 @@ fn empty_history_converges_trivially() {
|
|||||||
assert!(report.converged);
|
assert!(report.converged);
|
||||||
}
|
}
|
||||||
|
|
||||||
|
/// Issue #27's exact reproduction: a non-default key type reaching `converge`
|
||||||
|
/// with no events at all. The underflow it reported trapped in debug and
|
||||||
|
/// indexed out of bounds in release, so this must run in both profiles.
|
||||||
|
#[test]
|
||||||
|
fn converge_on_an_empty_history_with_owned_keys() {
|
||||||
|
let mut history: History<i64, ConstantDrift, NullObserver, String> =
|
||||||
|
History::builder_with_key().score_sigma(5.0).build();
|
||||||
|
|
||||||
|
let report = history.converge().unwrap();
|
||||||
|
|
||||||
|
assert_eq!(report.iterations, 0);
|
||||||
|
assert!(report.converged);
|
||||||
|
}
|
||||||
|
|
||||||
#[test]
|
#[test]
|
||||||
fn empty_event_stream_then_converge() {
|
fn empty_event_stream_then_converge() {
|
||||||
let mut h = History::default();
|
let mut h = History::default();
|
||||||
|
|||||||
Reference in New Issue
Block a user