From 965ea7ed3c966e030b3d033fea9b05367f2d29e9 Mon Sep 17 00:00:00 2001 From: Anders Olsson Date: Fri, 8 May 2026 15:23:11 +0200 Subject: [PATCH] =?UTF-8?q?docs:=20spec=20for=20History=20=E2=86=92=20Time?= =?UTF-8?q?Slice=20ConvergenceOptions=20plumbing?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- ...-08-history-convergence-plumbing-design.md | 232 ++++++++++++++++++ 1 file changed, 232 insertions(+) create mode 100644 docs/superpowers/specs/2026-05-08-history-convergence-plumbing-design.md diff --git a/docs/superpowers/specs/2026-05-08-history-convergence-plumbing-design.md b/docs/superpowers/specs/2026-05-08-history-convergence-plumbing-design.md new file mode 100644 index 0000000..2cb3702 --- /dev/null +++ b/docs/superpowers/specs/2026-05-08-history-convergence-plumbing-design.md @@ -0,0 +1,232 @@ +# History → TimeSlice ConvergenceOptions Plumbing + +## Summary + +Make `History`'s already-public `ConvergenceOptions` (set via +`HistoryBuilder::convergence(...)`) actually reach the within-game +inference loop. Today it's read by the outer `History::converge` sweep +but dropped on the floor when constructing `TimeSlice`s, so users who +opt in to `alpha < 1.0` (Damped EP) on a `History` get nothing — the +inner `run_chain` calls inside `TimeSlice` hardcode +`ConvergenceOptions::default()`. + +This spec closes the gap with one focused change: thread +`ConvergenceOptions` from `History` through `TimeSlice` to the three +`Game::*_with_arena` callsites in `time_slice.rs`. No new types, no new +public methods on `History` or `HistoryBuilder` — the user-facing API +already exists. + +## Background + +After T5 (commit `0705986`) of the Damped EP plan, +`Game::*_with_arena` accepts `convergence: ConvergenceOptions` and +`run_chain` reads `self.convergence.{epsilon, max_iter, alpha}`. +`HistoryBuilder` already has a `convergence(opts)` method (`history.rs:91`) +that stores onto a field on `History`. `History::converge` reads +`self.convergence.{max_iter, epsilon}` for its outer cross-history loop +(`history.rs:437-447`). + +The break is here, in `History::add_events_with_prior` at `history.rs:597`: + +```rust +let mut time_slice = TimeSlice::new(t, self.p_draw); +``` + +`self.convergence` is not passed. `TimeSlice` has no convergence field. +The three callsites in `time_slice.rs` that build `Game::*_with_arena` +fall back to `ConvergenceOptions::default()`: + +- `Event::iteration_direct` (`time_slice.rs:138-156`) +- `TimeSlice::convergence` (`time_slice.rs:332-345`) +- `TimeSlice::log_evidence` (`time_slice.rs:521-538`) + +## Scope + +### What ships + +1. `TimeSlice` gains a `pub(crate) convergence: ConvergenceOptions` + field set at construction. +2. `TimeSlice::new` signature becomes + `pub fn new(time: T, p_draw: f64, convergence: ConvergenceOptions) -> Self`. +3. `History::add_events_with_prior` (`history.rs:597`) passes + `self.convergence` when constructing new `TimeSlice`s. +4. `Event::iteration_direct` gains a `convergence: ConvergenceOptions` + parameter and forwards it to the `Game::*_with_arena` callsite. + The two callers (`TimeSlice::iteration` at `time_slice.rs:419` and + `:441`) pass `self.convergence`. +5. `TimeSlice::convergence` (the method, not the field) replaces its + hardcoded `crate::ConvergenceOptions::default()` with + `self.convergence`. +6. `TimeSlice::log_evidence` does the same. +7. Five test callsites of `TimeSlice::new(time, p_draw)` updated + mechanically to `TimeSlice::new(time, p_draw, ConvergenceOptions::default())`. + +### What does not ship + +- No split of `ConvergenceOptions` into outer/inner fields. The + conflation (one `max_iter` covers both the cross-history sweep and + the per-game EP iteration cap) is the user-confirmed design. +- No `Damped` impl in `src/schedule.rs`. The `Schedule` trait is still + not integrated into `run_chain`. +- No nat-param convergence switch. +- No oscillation auto-detect. +- No new `History` or `HistoryBuilder` methods. `convergence(opts)` + already exists and works. +- No changes to `History::converge` — the outer-loop semantics are + unchanged (it already reads `self.convergence`). + +## Design + +### `TimeSlice` field + +```rust +// src/time_slice.rs +pub struct TimeSlice { + // ... existing fields ... + p_draw: f64, + pub(crate) convergence: ConvergenceOptions, + // ... existing fields ... +} +``` + +### `TimeSlice::new` + +```rust +impl TimeSlice { + pub fn new(time: T, p_draw: f64, convergence: ConvergenceOptions) -> Self { + Self { + // ... existing initialisation ... + p_draw, + convergence, + // ... + } + } +} +``` + +### `History::add_events_with_prior` — single-line fix + +At `src/history.rs:597`: + +```rust +// before +let mut time_slice = TimeSlice::new(t, self.p_draw); + +// after +let mut time_slice = TimeSlice::new(t, self.p_draw, self.convergence); +``` + +### `Event::iteration_direct` parameter + +```rust +// src/time_slice.rs +impl Event { + pub(crate) fn iteration_direct( + &mut self, + skills: &mut SkillStore, + agents: &CompetitorStore, + p_draw: f64, + convergence: ConvergenceOptions, + arena: &mut ScratchArena, + ) -> /* existing return */ { + // ... existing body, with the Game::*_with_arena calls + // using `convergence` instead of ConvergenceOptions::default() ... + } +} +``` + +The two callers — `TimeSlice::iteration` at `time_slice.rs:419` and +`:441` — already have `&mut self` access, so they pass +`self.convergence`. + +### `TimeSlice::convergence` method (not the field) + +The method `pub(crate) fn convergence(&mut self, agents: ...) -> usize` +at `time_slice.rs:447` shares its name with the new field. Rust allows +this (methods and fields live in different namespaces), but it's a +readability hazard. Rename the method to `iterate_to_convergence` to +disambiguate. + +This is one rename, six callsites in `history.rs` and the test module. + +### Field semantics + +`History` keeps the single shared `ConvergenceOptions` struct. The same +`max_iter` covers both the outer sweep and each inner per-game loop. +The same `epsilon` covers both stopping criteria. The `alpha` field is +read only inside `run_chain` (the inner loop); the outer loop +intentionally ignores `alpha` because cross-history damping is a +different mathematical concept and not in scope. + +## Testing strategy + +### Regression net + +The existing 98 lib + 27 integration tests are the bit-equal regression +net. Default `ConvergenceOptions` is unchanged +(`max_iter=30, epsilon=1e-6, alpha=1.0`), and `TimeSlice` was already +using exactly that since T5. The only behavioural difference is for +users who actually pass non-default options through +`HistoryBuilder::convergence(...)` — and there are no current tests that +do that **and** compare posteriors, so all goldens stay bit-equal. + +### New tests + +1. **`history_propagates_convergence_to_inner_run_chain`** (in + `src/history.rs` test module): + - Build a History with `convergence(ConvergenceOptions { max_iter: 1, ..Default::default() })`. + - Add a small batch of events that needs more than one inner EP iteration to converge (e.g. a 4-team game per slice). + - `converge()`, capture posteriors. + - Build a fresh History with default options on the same events. + - `converge()`, capture posteriors. + - Assert the two sets of posteriors differ measurably (max diff > 1e-6). + - Proves the inner loop honours the propagated `max_iter`. Today (without this change) the assertion would fail because both Histories use default inside. + +2. **`history_with_damping_reaches_same_fixed_point_as_undamped`** (same + test module): + - Build a History with `convergence(ConvergenceOptions { alpha: 0.5, max_iter: 200, ..Default::default() })`. + - Same events as above. + - `converge()`, capture posteriors. + - Build a default-options History on the same events. + - `converge()`, capture posteriors. + - Assert per-player posteriors agree within 1e-3. + - Proves damping doesn't break convergence on the History path. + +If the second test's max diff is too large, raise `max_iter` further +(damping needs more iterations to reach the same fixed point). + +## Verification gates + +```bash +cargo +nightly fmt +cargo clippy --all-targets -- -D warnings +cargo test --lib +cargo test +``` + +All must succeed. Test count grows by exactly 2 (the two new tests). + +## Risks + +- **`TimeSlice::new` is `pub`.** Adding the third parameter is a + breaking change to a public constructor. In a 0.1.x crate this is + acceptable, but flag it in the commit message. +- **`TimeSlice::convergence` method rename.** Renaming + `convergence` → `iterate_to_convergence` touches `history.rs` and the + TimeSlice test module. The rename is mechanical and improves + readability where the field and method would otherwise share a name. +- **Cross-history alpha semantics.** A user who sets `alpha = 0.5` on + a `History` gets damping inside every per-game loop, but the outer + `History::converge` sweep is undamped. This is the correct semantic + (alpha is a within-EP-graph concept) but it's worth documenting in + the `ConvergenceOptions::alpha` doc comment so users don't expect + cross-slice damping. Add one sentence to the existing doc comment. + +## Out-of-scope follow-ups + +- Wire `Schedule` trait into `run_chain` — Damped becomes a `Schedule` + impl alongside `EpsilonOrMax`. +- Per-loop `ConvergenceOptions` split (outer / inner). +- `Residual` schedule. +- Per-event `EventKind::Scored.score_sigma` override (still + history-wide today).