Convergence configuration is inconsistent: two different iteration defaults, and converged: false for trivially-converged graphs #22

New Issue

2026-08-04T19:20:15Z

logaritmisk commented

2026-08-04 19:20:15 +00:00

Three unrelated-looking defects that all make convergence reporting untrustworthy.

1. Two different default iteration caps

ConvergenceOptions::default() → max_iter: crate::ITERATIONS = 30 (src/convergence.rs:26, src/lib.rs:55)
EpsilonOrMax::default() → max: 10 (src/schedule.rs:36), commented "Matches today's hard-coded tolerance and iteration cap"

Both are public defaults for "how many EP iterations before giving up", and they disagree by 3×. Which one applies depends on whether inference goes through run_chain (reads ConvergenceOptions, src/game.rs:279) or through a Schedule (EpsilonOrMax). A user who configures one and gets the other's behaviour has no way to tell from the API.

Pick one source of truth. Since run_chain is the path everything actually uses, EpsilonOrMax::default() should derive from ConvergenceOptions::default() — or the two should be unified outright (see #6, which needs Schedule generalised anyway).

2. `ScheduleReport.converged` is `false` when there was nothing to converge

EpsilonOrMax::run initialises converged = false and only sets it inside the iteration loop (src/schedule.rs:52-81). If the factor list contains no iterating factors — a graph of only TeamSum setup factors — the loop never runs and the report says converged: false with final_step: (INFINITY, INFINITY), despite the graph being trivially at its fixed point.

The existing test documents the behaviour rather than questioning it (src/schedule.rs:114-125):

// No iterating factors → 0 iterations, converged stays false (loop never ran).

A caller checking report.converged gets a false negative. A graph with nothing to iterate has converged.

3. `alpha` is documented as validated but isn't

ConvergenceOptions.alpha says "Must be in (0.0, 1.0]" (src/convergence.rs:14). Nothing enforces it outside a debug_assert! in the game constructors (src/game.rs:213, 252), and HistoryBuilder::convergence accepts the struct wholesale with no check (src/history.rs:91). Consequences in release:

alpha = 0.0 → damp_natural returns the old message unchanged every time (src/gaussian.rs:112), so no factor ever updates. The loop runs to max_iter and reports converged: false — but silently, and the posteriors are just the priors.
alpha > 1.0 → over-relaxation, which can diverge.

See #18 for the general validation problem; this specific one is worth calling out because the failure mode is "silently returns the prior".

Also

ScheduleReport.final_step is (f64, f64) throughout — a bare tuple whose meaning (|Δmu|, |Δsigma|) lives only in a doc comment on Factor::propagate (src/factor/mod.rs:62). Given #15 proposes switching these to (|Δpi|, |Δtau|), a named type would prevent the units silently changing meaning under callers.

Acceptance

One documented default iteration cap, used by every path.
A graph with no iterating factors reports converged: true.
Out-of-range alpha is rejected at the API boundary in release builds.

Three unrelated-looking defects that all make convergence reporting untrustworthy. ## 1. Two different default iteration caps - `ConvergenceOptions::default()` → `max_iter: crate::ITERATIONS` = **30** (`src/convergence.rs:26`, `src/lib.rs:55`) - `EpsilonOrMax::default()` → `max: 10` (`src/schedule.rs:36`), commented "Matches today's hard-coded tolerance and iteration cap" Both are public defaults for "how many EP iterations before giving up", and they disagree by 3×. Which one applies depends on whether inference goes through `run_chain` (reads `ConvergenceOptions`, `src/game.rs:279`) or through a `Schedule` (`EpsilonOrMax`). A user who configures one and gets the other's behaviour has no way to tell from the API. Pick one source of truth. Since `run_chain` is the path everything actually uses, `EpsilonOrMax::default()` should derive from `ConvergenceOptions::default()` — or the two should be unified outright (see #6, which needs `Schedule` generalised anyway). ## 2. `ScheduleReport.converged` is `false` when there was nothing to converge `EpsilonOrMax::run` initialises `converged = false` and only sets it inside the iteration loop (`src/schedule.rs:52-81`). If the factor list contains no iterating factors — a graph of only `TeamSum` setup factors — the loop never runs and the report says `converged: false` with `final_step: (INFINITY, INFINITY)`, despite the graph being trivially at its fixed point. The existing test documents the behaviour rather than questioning it (`src/schedule.rs:114-125`): ```rust // No iterating factors → 0 iterations, converged stays false (loop never ran). ``` A caller checking `report.converged` gets a false negative. A graph with nothing to iterate has converged. ## 3. `alpha` is documented as validated but isn't `ConvergenceOptions.alpha` says "Must be in `(0.0, 1.0]`" (`src/convergence.rs:14`). Nothing enforces it outside a `debug_assert!` in the game constructors (`src/game.rs:213`, `252`), and `HistoryBuilder::convergence` accepts the struct wholesale with no check (`src/history.rs:91`). Consequences in release: - `alpha = 0.0` → `damp_natural` returns the old message unchanged every time (`src/gaussian.rs:112`), so no factor ever updates. The loop runs to `max_iter` and reports `converged: false` — but silently, and the posteriors are just the priors. - `alpha > 1.0` → over-relaxation, which can diverge. See #18 for the general validation problem; this specific one is worth calling out because the failure mode is "silently returns the prior". ## Also `ScheduleReport.final_step` is `(f64, f64)` throughout — a bare tuple whose meaning (`|Δmu|`, `|Δsigma|`) lives only in a doc comment on `Factor::propagate` (`src/factor/mod.rs:62`). Given #15 proposes switching these to `(|Δpi|, |Δtau|)`, a named type would prevent the units silently changing meaning under callers. ## Acceptance - One documented default iteration cap, used by every path. - A graph with no iterating factors reports `converged: true`. - Out-of-range `alpha` is rejected at the API boundary in release builds.

logaritmisk referenced this issue

2026-08-04 19:20:44 +00:00

Dead and duplicated code: a stale `iterate_to_convergence`, a copy-pasted sweep, and `#[allow(dead_code)]` used as a mute button #23

logaritmisk referenced this issue

2026-08-04 20:11:51 +00:00

Hot-path Gaussian arithmetic round-trips through sqrt; convergence deltas computed in moment space #15

logaritmisk commented

2026-08-04 20:12:07 +00:00

Fixed in 6030dc7 — all three.

EpsilonOrMax::default() derives from ConvergenceOptions::default(), so there is one source of truth. Pinned by default_matches_convergence_options.
A graph with no iterating factors now reports converged: true with a zero step. The test that documented the old false negative asserts the new behaviour.
HistoryBuilder::convergence rejects an alpha outside (0.0, 1.0] and a negative epsilon, matching the eager-panic convention score_sigma already used; p_draw validates too. An alpha of zero was the nastiest of these — every EP update goes unapplied, so inference silently returns the priors.

TimeSlice::iterate_to_convergence was a fourth source of disagreement — hard-coded epsilon and a 20-iteration cap matching neither default. It reads self.convergence now and is #[cfg(test)], which is all that ever called it. Two goldens moved as a result: they are convergence residuals, and running to 30 iterations instead of 20 lands nearer the analytic truth of 25.0.

Still open (kept in this issue): final_step is still a bare (f64, f64) tuple, and the deltas are still moment-space rather than (|Δpi|, |Δtau|) per spec §5 — see #15, item 3. A named type is worth having before those units change meaning under callers.

Fixed in 6030dc7 — all three. 1. `EpsilonOrMax::default()` derives from `ConvergenceOptions::default()`, so there is one source of truth. Pinned by `default_matches_convergence_options`. 2. A graph with no iterating factors now reports `converged: true` with a zero step. The test that documented the old false negative asserts the new behaviour. 3. `HistoryBuilder::convergence` rejects an `alpha` outside `(0.0, 1.0]` and a negative epsilon, matching the eager-panic convention `score_sigma` already used; `p_draw` validates too. An `alpha` of zero was the nastiest of these — every EP update goes unapplied, so inference silently returns the priors. `TimeSlice::iterate_to_convergence` was a fourth source of disagreement — hard-coded epsilon and a 20-iteration cap matching neither default. It reads `self.convergence` now and is `#[cfg(test)]`, which is all that ever called it. Two goldens moved as a result: they are convergence residuals, and running to 30 iterations instead of 20 lands nearer the analytic truth of 25.0. **Still open** (kept in this issue): `final_step` is still a bare `(f64, f64)` tuple, and the deltas are still moment-space rather than `(|Δpi|, |Δtau|)` per spec §5 — see #15, item 3. A named type is worth having *before* those units change meaning under callers.

logaritmisk closed this issue

2026-08-04 20:13:43 +00:00

logaritmisk added the api bug labels 2026-09-07 13:53:24 +00:00

logaritmisk referenced this issue

2026-09-07 21:27:19 +00:00

`ITERATIONS = 30` is far too low for a dense history, and overrunning it fails quietly #50

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: logaritmisk/trueskill-tt#22

Convergence configuration is inconsistent: two different iteration defaults, and converged: false for trivially-converged graphs #22

1. Two different default iteration caps

2. ScheduleReport.converged is false when there was nothing to converge

3. alpha is documented as validated but isn't

Also

Acceptance

Convergence configuration is inconsistent: two different iteration defaults, and `converged: false` for trivially-converged graphs #22

2. `ScheduleReport.converged` is `false` when there was nothing to converge

3. `alpha` is documented as validated but isn't