61da3aca33eb3df1ab68064d3d65d2ffa903b0cd
5
Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
  logaritmiskandClaude Opus 5
|
a18df521eb |
test: cover non-finite results and color-group disjointness
The two gaps #26 named that were never filled. NonFiniteResult had no test at all — the name appeared in `tests/` only inside a doc comment, and it is the sub-claim in that issue's title. It turns out to be very much reachable, and from *finite* inputs: sigma at 1e300, beta at 1e300, sigma at 1e-300, score_sigma at 1e-300, and scores at 1e308 all overflow inside inference, where the boundary checks cannot see them. That matters because the failure is silent by default — NaN fails every comparison, so a naive `step < epsilon` reads a NaN step as converged, which is why the crate has `step_converged`/`step_is_finite`. Pinned from outside, including that `converge_partial` does not launder a breakdown into an `Ok`, and with a control asserting merely extreme parameters still converge so the suite cannot pass by always failing. Color-group disjointness was #26's fourth acceptance criterion and had only five hand-written cases. Now a proptest over three shapes: a dense pool where collisions force colors to multiply, a sparse one where most events are independent, and repeated members within a single event. Two of my first assertions were wrong about the code rather than the reverse. A competitor named twice *within* one event is not a collision — `color_greedy` collects each event's members into a set for that reason. And contiguity is not a property of `color_greedy`: it holds only after `recompute_color_groups` reorders events so each color occupies one range. The test now asserts what is actually promised — that the reorder is always *possible*, since the parallel sweep slices `&mut` sub-ranges from those groups and overlapping ranges would be unsound. Refs #26 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011hcFjNDmHXZF8URGLku5zZ |
||
|
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 |
||
|
logaritmiskandClaude Opus 5
|
6030dc78de |
refactor: unify convergence defaults, validate builders, clear dead code
Convergence configuration had two disagreeing sources of truth and one
misleading report:
- `EpsilonOrMax::default()` capped at 10 iterations while
`ConvergenceOptions::default()` allowed 30, and which applied depended on
whether inference went through `run_chain` or a `Schedule`. The schedule
default now derives from `ConvergenceOptions`.
- A graph with no iterating factors reported `converged: false` with an
infinite step, despite being at its fixed point after the setup pass. It
now reports converged with a zero step.
- `TimeSlice::iterate_to_convergence` hard-coded an epsilon and a
20-iteration cap matching neither. It reads `self.convergence` and is
scoped to `#[cfg(test)]`, which is all it was ever used by.
`HistoryBuilder::p_draw` and `::convergence` now validate their arguments
like `score_sigma` already did, instead of accepting a negative `p_draw` or
an `alpha` of zero — the latter leaves every EP update unapplied, so
inference silently returns the priors.
Removing the `#[allow(dead_code)]` masks let the compiler report what they
were hiding: four `OwnedGame` fields that were stored and never read, two
`ColorGroups` helpers and three `SkillStore` helpers used only by tests, and
`iterate_to_convergence` above. Test-only items are now `#[cfg(test)]` and
the unread fields are gone.
Also exported `HistoryBuilder`, which was public but unreachable — callers
could chain `History::builder()` but could not name the type — and added
`Rating::{prior, beta, drift}` and `Index::get`, so handles the API hands
out can be read back.
Two goldens moved, both convergence residuals rather than exact values:
`iterate_to_convergence` now runs to 30 iterations instead of 20, landing
nearer the symmetric truth of 25.0.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01DnsaJg74eNSva3PJjK2eej
|
||
|
logaritmiskandClaude Opus 5
|
c088214fed |
fix(history): stop reprocessing the slice that was just appended to
`add_events_with_prior` advanced `k` past the slice it had written when it
created a new one, but not when it appended to an existing one. The trailing
forward-refresh loop therefore started *on* the slice just modified and ran
`new_forward_info` over it again.
That is not merely redundant work. The loop immediately above it sets each
agent's message to `forward * likelihood` for that slice, and
`new_forward_info` then assigns `skill.forward = message.forget(drift)` —
folding the slice's own likelihood back into its own forward prior. The
skills it produced depended on how events had been batched.
Ingesting one event at a time now converges to the same fixed point as
ingesting the same events in a single call, which it previously did not:
for five events sharing a timestamp, competitor `a` converged to
mu=7.44 sigma=3.90 batched versus mu=7.99 sigma=3.10 incrementally. Both
runs had converged; the gap was not a convergence residual.
The numerical goldens never caught this because they all ingest in one call
with a distinct timestamp per event, so the append-to-existing-slice branch
is never taken. `tests/ingestion_equivalence.rs` covers it directly, and
asserts convergence before comparing so that a residual cannot be mistaken
for agreement.
Removing the redundant re-inference also removes the dominant cost of
incremental ingestion, which was quadratic in the number of events already
in the slice:
events before after speedup
500 45.8ms 1.1ms 42x
1000 179.5ms 2.8ms 64x
2000 721.8ms 9.9ms 73x
4000 2.9s 35.4ms 82x
Ingesting one at a time is now 1.8x a single batched call, down from 148x.
Two supporting changes are included:
- Color groups are rebuilt lazily rather than on every append. Nothing
reads the partition between an append and the next full sweep, so the
per-append rebuild was pure waste.
- `ColorGroups::groups_are_contiguous` is asserted after each rebuild and
in `color_range`. The parallel sweep derives one `&mut` sub-slice per
color from those ranges and relies on them being disjoint; that invariant
was established by construction but never checked.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01DnsaJg74eNSva3PJjK2eej
|
||
|
logaritmisk
|
6bf3e7e294 |
T3: rayon-backed concurrency (opt-in) (#2)
Implements T3 of `docs/superpowers/specs/2026-04-23-trueskill-engine-redesign-design.md` Section 6. Plan: `docs/superpowers/plans/2026-04-24-t3-concurrency.md` (11 tasks).
## Summary
### Breaking
- `Send + Sync` bounds added to public traits: `Time`, `Drift<T>`, `Observer<T>`, `Factor`, `Schedule`. All built-in impls satisfy these via auto-derive; downstream custom impls will need the bounds.
### New
- Opt-in `rayon` cargo feature. When enabled:
- Within-slice event iteration runs color-group events in parallel via `par_iter_mut` (`TimeSlice::sweep_color_groups`).
- `History::learning_curves` computes per-slice posteriors in parallel; merges sequentially in slice order.
- `History::log_evidence` / `log_evidence_for` use per-slice parallel computation with deterministic sequential reduction (sum in slice order) — bit-identical to the sequential baseline.
- `ColorGroups` infrastructure (`src/color_group.rs`) with greedy graph coloring. Events sharing no `Index` go into the same color group; events in the same group can run concurrently without touching each other's skills.
- `tests/determinism.rs` asserts bit-identical posteriors across `RAYON_NUM_THREADS={1, 2, 4, 8}`.
- `benches/history_converge.rs` measures end-to-end convergence on three workload shapes.
## Performance
### Sequential (no rayon, default build)
| Metric | Before T3 | After T3 | Delta |
|---|---|---|---|
| `Batch::iteration` | 22.88 µs | 23.23 µs | **+1.5%** (noise) |
| `Gaussian::*` | ≈218–264 ps | ≈236 ps | within noise |
**No sequential regression.** Default build is as fast as T2.
### Parallel (`--features rayon`, Apple M5 Pro, auto thread count)
| Workload | Sequential | Parallel | Speedup |
|---|---:|---:|---:|
| 500 events / 100 competitors / 10 per slice | 4.03 ms | 4.24 ms | **1.0×** |
| 2000 events / 200 competitors / 20 per slice | 20.18 ms | 19.82 ms | **1.0×** |
| 5000 events / 50000 competitors / 1 slice | 11.88 ms | 9.10 ms | **1.3×** |
### ⚠️ The spec's >=2× target was not met on realistic workloads.
T3's within-slice color-group parallelism only shows material benefit when a slice holds many events AND the competitor pool is large enough to give the greedy coloring room to partition. Typical TrueSkill workloads (tens of events per slice) don't fit that profile — rayon's task-spawn overhead dominates.
**Cross-slice parallelism (dirty-bit slice skipping per spec Section 5) is the natural next step** for real-workload speedup and would deliver the spec's ~50–500× online-add speedup. Deferred to a future tier.
## Determinism
`tests/determinism.rs` runs a 200-event history at thread counts {1, 2, 4, 8} via `rayon::ThreadPoolBuilder::install` and asserts every `(time, posterior)` pair has bit-identical `mu` and `sigma` (compared via `f64::to_bits()`). Passes.
## Internals
- Parallel path uses an `unsafe` block to concurrently write to `SkillStore` from color-group-disjoint events. Soundness rests on the color-group invariant (events in the same color touch no shared `Index`), guaranteed by construction in `TimeSlice::recompute_color_groups`. Sequential path unchanged from T2.
- `RAYON_THRESHOLD = 64` — color groups smaller than this fall back to sequential inside `sweep_color_groups` to avoid task-spawn overhead.
- Thread-local `ScratchArena` per rayon worker thread.
## Test plan
- [x] `cargo test --features approx` — 96 tests pass (74 lib + 22 integration)
- [x] `cargo test --features approx,rayon` — 97 tests pass (+1 determinism)
- [x] `cargo clippy --all-targets --features approx -- -D warnings` — clean
- [x] `cargo clippy --all-targets --features approx,rayon -- -D warnings` — clean
- [x] `cargo +nightly fmt --check` — clean
- [x] `cargo bench --bench batch --features approx` — 23.23 µs (no regression vs T2)
- [x] `cargo bench --bench history_converge --features approx,rayon` — runs on all three workloads
- [x] Bit-identical posteriors across `RAYON_NUM_THREADS={1, 2, 4, 8}` — verified
## Commit history
13 commits on `t3-concurrency`. Each task is self-contained and bisectable. See `git log main..t3-concurrency` for the full list.
## Deferred
- **Cross-slice parallelism** (dirty-bit slice skipping) — the path that would actually speed up typical TrueSkill workloads.
- **Default-on `rayon` feature** — spec called for default-on; we keep it opt-in until the feature proves stable in production use.
- **Synchronous-EP schedule with barrier merge** — alternative parallel strategy per spec Section 6.
- **`MarginFactor` / `Outcome::Scored`** — T4.
- **`Damped` / `Residual` schedules** — T4.
- **N-team `predict_outcome`** — T4.
- **`Game::custom` full ergonomics** — T4.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Reviewed-on: #2
Co-authored-by: Anders Olsson <anders.e.olsson@gmail.com>
Co-committed-by: Anders Olsson <anders.e.olsson@gmail.com>
|