Issue #5 asked for a decision, not an implementation: either flip rayon
to default-on, or record why the spec was deviated from and close.
Opt-in stands. The measured speedups are 1.0x realistic / 1.3x
pathological (#4), so default-on would cost every downstream user a
thread pool and a dependency for approximately nothing.
The condition the decision was waiting on cannot be met: #5 was blocked
on re-measuring after cross-slice dirty-bit skipping landed, and #4 was
closed by removing the inert slices_skipped field rather than by
implementing it. There is no forthcoming measurement to wait for.
Also corrects the spec's own reasoning. It cited an unsafe concurrent
write through SkillStore as a cost of going default-on; the crate is
forbid(unsafe_code) and the compute/apply split avoids that entirely.
The case for opt-in is the measurements, not a safety argument.
Closes#5
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011hcFjNDmHXZF8URGLku5zZ
#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
Five tasks: delete the inert online machinery, add filtered_log_evidence,
add the two learning-curve methods, pin the invariants, record the API break.
Two spec corrections fell out of writing it. The spec claimed filtered results
would be bit-identical before and after converge(); they cannot be. iteration
recomputes the colour partition only when from == 0, so a slice built by
repeated appends keeps insertion order until the first converge() reorders it,
and the scratch clone inherits whichever order it finds — same fixed point,
different path. Corrected to agreement within 1e-8 under tight convergence,
matching the house pattern in tests/ingestion_equivalence.rs. The spec also
declared filtered_pass as Vec<(T, Vec<(Index, Gaussian)>)>, which cannot carry
the evidence its own step 3 harvests; it returns Vec<(T, FilteredStep)>.
CHANGELOG.md is generated by git-cliff, so the spec's "CHANGELOG records the
API break" cannot be satisfied by editing the file — it regenerates. Task 5
records the break through the commit subject and verifies the generated output
instead. cliff.toml has no breaking-change parser at all, which the task is
told to report rather than work around.
An adversarial reviewer checked the plan against the source before this commit
and found four real defects, all in plan text, none in the design:
- Two prescribed mutations provably could not fail their named tests. The
learning-curve mutation altered only what filtered_pass writes after a slice,
while the test inspected filtered[0], which is computed from an empty message
map. Fixed by asserting monotonic mu across the whole curve.
- The ingestion-order fixture used four distinct timestamps, giving one event
per slice — the exact degenerate shape ingestion_equivalence.rs documents as
the weak case, making the assertion true by construction. Fixed to several
events per timestamp with shared competitors.
- filtered_learning_curves was never asserted for content, only for emptiness
on an empty history.
- A doc comment restated learning_curves' claim that key(idx) is O(n) and the
method O(n^2). KeyTable::key is self.reverse.get(idx.0) — O(1) — and the
type's own doc says so. The claim predates reverse becoming a Vec. The plan
now corrects the original at history.rs:323 rather than copying it.
The reviewer confirmed the central claim by tracing the call graph: N_INF is
{pi: 0, tau: 0} and Mul is a natural-parameter add, so it is an exact
multiplicative identity, and the only write to skill.backward in the crate is
in new_backward_info, reachable only from History::iteration and never from
iterate_to_convergence under either rayon cfg.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01T5SYDExxL4vZgvunrcNSMc
`HistoryBuilder::online(true)` is inert: it flips a flag that reaches
`Item::within_prior`, which reads `Skill.online` — a field initialised to
`N_INF` and assigned nowhere. So `log_evidence()` under that setting reports
`n * ln(0.5)`, every game scored as a coin flip. The number is finite and
plausible, which is why nothing caught it.
Issue #19 proposed populating the field during the forward pass. That does
not work, and the reason shapes the whole design. `new_forward_info` sets
`skill.forward` from the previous slice's `forward_prior_out`, which is
`skill.forward * skill.likelihood`; `History::iteration` alternates backward
and forward sweeps, so from the second iteration onward that likelihood has
already absorbed backward information. After `converge()`, `skill.forward`
is a smoothed quantity — and so is anything written from it.
The same reasoning condemns the neighbouring `forward: bool` flag, which is
a filtering quantity only on a history that was never converged. That is why
the test at history.rs:1183 can assert the two evidences are equal. Left
alone here; recorded as a follow-up.
The design is a read-only forward-only pass instead: walk slices in time
order carrying their own forward messages, and per slice build a scratch
clone whose `backward` is `N_INF`, then run the unmodified production sweep
on it. Reusing `iterate_to_convergence` rather than reimplementing inference
means a competitor playing twice at one time is handled by the same
within-slice EP that `converge()` uses, instead of being approximated the
way today's evidence paths approximate it. Nothing is stored on `Skill`,
which drops 16 bytes and helps #17 regardless.
Three methods ship — `filtered_log_evidence`, `filtered_learning_curves`,
`filtered_learning_curve` — all taking `&self`. The second consumer is
ustat, whose learning curves start already collapsed to sigma 0.9-1.6
against a prior of 6.0 because every point is smoothed; the filtered view
cannot be reconstructed from the public API today except by O(n^2) refits.
The red test brackets the issue's own fixture strictly between 5*ln(0.5) and
the batch evidence, so neither "still inert" nor "accidentally smoothed"
passes. The invariant that would have caught this bug class is that filtered
results are identical before and after `converge()` — exactly what a stored
field cannot give.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01T5SYDExxL4vZgvunrcNSMc
Outcome::Scored becomes a struct variant with an Option<f64> sigma
field. None inherits HistoryBuilder::score_sigma; Some(s) overrides
per event. Resolved at ingest time so EventKind::Scored stays a plain
f64 and TimeSlice/run_chain need zero changes. New constructors
Outcome::scores_with_sigma and EventBuilder::scores_with_sigma cover
the override path; existing scores(..) keeps its signature with
sigma=None internally.
Breaking change to Outcome::Scored variant shape (tuple → struct);
acceptable in 0.1.x. Closes the last item from the T4-MarginFactor
deferred wishlist.
Closes the gap between HistoryBuilder::convergence(opts) and the
within-game inference loop. TimeSlice gains a convergence field;
History passes self.convergence at construction; the three
Game::*_with_arena callsites in time_slice.rs read it. Also renames
TimeSlice::convergence the method (now iterate_to_convergence) to
disambiguate from the new field.
Pure plumbing — no new public API, no behavioral change for users on
default options. Makes Damped EP reachable through the History path.
Six tasks: Gaussian::damp_natural helper, ConvergenceOptions::alpha
field, TruncFactor and MarginFactor propagate_with_alpha pair, DiffFactor
+ Game integration (the big task — must land atomically), and
end-to-end tests for max_iter and alpha behavior.
Smallest-scope realisation of spec §"Built-in schedules" Damped: a
ConvergenceOptions::alpha field plumbed through run_chain to a new
Gaussian::damp_natural helper applied inside TruncFactor and
MarginFactor's propagate. alpha=1.0 default keeps every existing
golden bit-equal; alpha<1.0 stabilises oscillating fixed-point loops
on hard graphs.
Defers Schedule trait integration, nat-param convergence switch,
oscillation auto-detect, Residual/OneShot, and Synergy/ScoreFactor —
each gets its own future plan.
The plan's prose quoted Z_cav ≈ 0.046827 and log_evidence ≈ -3.0613,
which diverged from the values asserted by the shipped test in
src/factor/mod.rs (-3.062235327364623). Update prose and the matching
code comment to 0.04678 / -3.0622.
Three independent cleanups: dedupe Game::likelihoods and likelihoods_scored
via a run_chain helper taking a make_link closure, make BuiltinFactor's
log_evidence match exhaustive, and fix stale numerics in the T4 plan doc.
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>
logaritmiskandClaude Opus 5