docs: spec for History → TimeSlice ConvergenceOptions plumbing
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.
This commit is contained in:
@@ -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<T>` 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<T>` field
|
||||
|
||||
```rust
|
||||
// src/time_slice.rs
|
||||
pub struct TimeSlice<T: Time = i64> {
|
||||
// ... existing fields ...
|
||||
p_draw: f64,
|
||||
pub(crate) convergence: ConvergenceOptions,
|
||||
// ... existing fields ...
|
||||
}
|
||||
```
|
||||
|
||||
### `TimeSlice::new`
|
||||
|
||||
```rust
|
||||
impl<T: Time> TimeSlice<T> {
|
||||
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<i64, ConstantDrift>,
|
||||
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<D>(&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).
|
||||
Reference in New Issue
Block a user