Files
trueskill-tt/src/color_group.rs
T
logaritmiskandClaude Opus 5 eeb43e3be1 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 in f4e2922 and the issue was filed
against 7742b2b, 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
2026-08-27 17:21:05 +02:00

194 lines
5.9 KiB
Rust
Raw Blame History

This file contains ambiguous Unicode characters
This file contains Unicode characters that might be confused with other characters. If you think that this is intentional, you can safely ignore this warning. Use the Escape button to reveal them.
//! Greedy graph coloring for within-slice event independence.
//!
//! Events sharing no `Index` can be processed in parallel under async-EP
//! semantics. This module partitions a list of events into "colors" such
//! that events of the same color touch disjoint index sets.
//!
//! The algorithm is greedy: for each event in ingestion order, place it in
//! the lowest-numbered color whose existing members share no `Index`. If
//! no existing color accepts the event, open a new color.
//!
//! Complexity: O(n × c × m) where n is events, c is colors (small, ≤ 5 in
//! practice), and m is average team size.
use std::collections::HashSet;
use crate::Index;
/// Partition of event indices into color groups.
///
/// Each inner `Vec<usize>` holds the indices (into the original events
/// array) of events assigned to one color. Colors are iterated in ascending
/// order by convention.
#[derive(Clone, Debug, Default)]
pub(crate) struct ColorGroups {
pub(crate) groups: Vec<Vec<usize>>,
}
impl ColorGroups {
pub(crate) fn new() -> Self {
Self::default()
}
pub(crate) fn is_empty(&self) -> bool {
self.groups.is_empty()
}
/// Number of distinct colors in the partition. Test-only.
#[cfg(test)]
pub(crate) fn n_colors(&self) -> usize {
self.groups.len()
}
/// Total event count across all colors. Test-only.
#[cfg(test)]
pub(crate) fn total_events(&self) -> usize {
self.groups.iter().map(|g| g.len()).sum()
}
/// Contiguous index range for one color after events have been reordered
/// into color-contiguous positions by `TimeSlice::recompute_color_groups`.
pub(crate) fn color_range(&self, color_idx: usize) -> std::ops::Range<usize> {
let group = &self.groups[color_idx];
if group.is_empty() {
return 0..0;
}
let start = *group.first().unwrap();
let end = *group.last().unwrap() + 1;
debug_assert_eq!(
end - start,
group.len(),
"color {color_idx} is not contiguous; its range would overlap other colors"
);
start..end
}
/// Whether every color occupies a contiguous, ascending range of event
/// indices, and no two colors overlap.
///
/// The parallel sweep derives one `&mut` sub-slice per color from these
/// ranges and relies on them being disjoint. That disjointness is what
/// makes concurrent writes to distinct skills sound, so it is checked
/// rather than assumed.
pub(crate) fn groups_are_contiguous(&self) -> bool {
let mut expected_start = 0;
for group in &self.groups {
if group.is_empty() {
continue;
}
let ascending_run = group
.iter()
.enumerate()
.all(|(offset, &idx)| idx == group[0] + offset);
if !ascending_run || group[0] != expected_start {
return false;
}
expected_start += group.len();
}
true
}
}
/// Compute color groups greedily.
///
/// `index_set(ev_idx)` yields, for each event index, the iterator of
/// `Index` values that event touches. The returned `ColorGroups` has one
/// inner `Vec<usize>` per color, containing event indices in the order
/// they were assigned.
pub(crate) fn color_greedy<I, F>(n_events: usize, index_set: F) -> ColorGroups
where
F: Fn(usize) -> I,
I: IntoIterator<Item = Index>,
{
let mut groups: Vec<Vec<usize>> = Vec::new();
let mut members: Vec<HashSet<Index>> = Vec::new();
for ev_idx in 0..n_events {
let ev_members: HashSet<Index> = index_set(ev_idx).into_iter().collect();
// Find first color whose member-set is disjoint from this event's indices.
let chosen = members.iter().position(|m| m.is_disjoint(&ev_members));
let color_idx = match chosen {
Some(c) => c,
None => {
groups.push(Vec::new());
members.push(HashSet::new());
groups.len() - 1
}
};
groups[color_idx].push(ev_idx);
members[color_idx].extend(ev_members);
}
ColorGroups { groups }
}
#[cfg(test)]
mod tests {
use super::*;
fn idx(i: usize) -> Index {
Index::from(i)
}
#[test]
fn single_event_gets_one_color() {
let cg = color_greedy(1, |_| vec![idx(0), idx(1)]);
assert_eq!(cg.n_colors(), 1);
assert_eq!(cg.groups[0], vec![0]);
}
#[test]
fn disjoint_events_share_a_color() {
let cg = color_greedy(2, |i| match i {
0 => vec![idx(0), idx(1)],
1 => vec![idx(2), idx(3)],
_ => unreachable!(),
});
assert_eq!(cg.n_colors(), 1);
assert_eq!(cg.groups[0], vec![0, 1]);
}
#[test]
fn overlapping_events_need_separate_colors() {
let cg = color_greedy(2, |i| match i {
0 => vec![idx(0), idx(1)],
1 => vec![idx(1), idx(2)],
_ => unreachable!(),
});
assert_eq!(cg.n_colors(), 2);
assert_eq!(cg.groups[0], vec![0]);
assert_eq!(cg.groups[1], vec![1]);
}
#[test]
fn three_events_two_colors() {
// Event 0: {0, 1}; event 1: {2, 3}; event 2: {0, 2}.
// Greedy: ev0→c0, ev1→c0 (disjoint), ev2 overlaps both→c1.
let cg = color_greedy(3, |i| match i {
0 => vec![idx(0), idx(1)],
1 => vec![idx(2), idx(3)],
2 => vec![idx(0), idx(2)],
_ => unreachable!(),
});
assert_eq!(cg.n_colors(), 2);
assert_eq!(cg.groups[0], vec![0, 1]);
assert_eq!(cg.groups[1], vec![2]);
}
#[test]
fn total_events_counts_correctly() {
let cg = color_greedy(4, |_| vec![idx(0)]);
// All events touch index 0 → 4 distinct colors.
assert_eq!(cg.n_colors(), 4);
assert_eq!(cg.total_events(), 4);
}
}