tuple_max erases NaN, so "NaN is never convergence" is a coin flip across processes #58

New Issue

2026-09-09T14:51:22Z

logaritmisk commented

2026-09-09 14:51:22 +00:00

src/lib.rs:572. The crate's stated invariant is "NaN is never convergence". step_converged and step_is_finite are each individually correct — the defect is one level up, in the reduction that produces the step they are handed.

pub(crate) fn tuple_max(v1: (f64, f64), v2: (f64, f64)) -> (f64, f64) {
    (
        if v1.0 > v2.0 { v1.0 } else { v2.0 },
        if v1.1 > v2.1 { v1.1 } else { v2.1 },
    )
}

Every call site is tuple_max(accumulator, new). NaN > x is false, so a NaN accumulator is discarded in favour of any later finite delta. A NaN reaches step only if the competitor that produced it happens to be reduced last.

It is folded over TimeSlice::posteriors(), a HashMap<Index, Gaussian> (src/time_slice.rs:382). Rust's default hasher is seeded per process, so "last" is random per run.

Measured

Four competitors in one slice: one pathological pair (Member::with_prior(Gaussian::from_ms(0.0, 1e-200))) and one healthy pair. Same binary, same input, 30 separate processes:

16  Ok  converged=true, iterations=1, a = Gaussian { pi: NaN, tau: NaN }
14  Err NonFiniteResult

A coin flip on whether a NaN fit is reported as an error or as a successful, converged fit. In the Ok half, converged: true is returned alongside a NaN posterior and current_skill hands the NaN straight back.

This is the defect class this crate keeps producing — a public surface reporting a plausible-looking answer — sitting inside the guard that exists to prevent it. It also means the two-competitor case erroring is luck rather than design: with a single pair there is nothing finite to overwrite the NaN with.

Affected call sites

src/game.rs:310,313,324,327, src/history.rs:407,431,448, src/time_slice.rs:565.

Fix

tuple_max must propagate NaN. f64::max is not the fix — it also ignores NaN by design (f64::max(NaN, 1.0) == 1.0), which is the identical bug wearing a standard-library name. It needs an explicit test:

fn max_propagating_nan(a: f64, b: f64) -> f64 {
    if a.is_nan() || b.is_nan() { f64::NAN } else if a > b { a } else { b }
}

Once the reduction propagates, step_is_finite fires deterministically and the existing guards do what they document.

Test that would have caught it

A NaN reduced from a non-final position. Existing coverage only ever produces a NaN that is either the whole fit or the last term — precisely the case that passes. Pin tuple_max directly at all three orderings (NaN first, middle, last) rather than only end to end, since the end-to-end version is the one that flakes and would be a maddening intermittent failure in CI.

Related, worth doing in the same pass

src/game.rs:301 — while tuple_gt(step, epsilon) && iter < max_iter is the only one of the three convergence loops with no step_is_finite break beside it; history.rs:1627 and time_slice.rs:557 both have one. Note an A/B measurement showed swapping in !step_converged there makes a 4-team chain run 20 000 iterations instead of 4 while producing bit-identical posteriors, so the fix there belongs in Gaussian::delta (returning 0.0 for the sigma component when both operands are improper), not in the loop condition.

Found by a floating-point audit, 2026-09-09. Verified independently of the agent that reported it.

`src/lib.rs:572`. The crate's stated invariant is "NaN is never convergence". `step_converged` and `step_is_finite` are each individually correct — the defect is one level up, in the reduction that produces the step they are handed. ```rust pub(crate) fn tuple_max(v1: (f64, f64), v2: (f64, f64)) -> (f64, f64) { ( if v1.0 > v2.0 { v1.0 } else { v2.0 }, if v1.1 > v2.1 { v1.1 } else { v2.1 }, ) } ``` Every call site is `tuple_max(accumulator, new)`. `NaN > x` is false, so **a NaN accumulator is discarded in favour of any later finite delta**. A NaN reaches `step` only if the competitor that produced it happens to be reduced last. It is folded over `TimeSlice::posteriors()`, a `HashMap<Index, Gaussian>` (`src/time_slice.rs:382`). Rust's default hasher is seeded per process, so "last" is random per run. ## Measured Four competitors in one slice: one pathological pair (`Member::with_prior(Gaussian::from_ms(0.0, 1e-200))`) and one healthy pair. Same binary, same input, 30 separate processes: ``` 16 Ok converged=true, iterations=1, a = Gaussian { pi: NaN, tau: NaN } 14 Err NonFiniteResult ``` **A coin flip on whether a NaN fit is reported as an error or as a successful, converged fit.** In the `Ok` half, `converged: true` is returned alongside a NaN posterior and `current_skill` hands the NaN straight back. This is the defect class this crate keeps producing — a public surface reporting a plausible-looking answer — sitting inside the guard that exists to prevent it. It also means the two-competitor case erroring is luck rather than design: with a single pair there is nothing finite to overwrite the NaN with. ## Affected call sites `src/game.rs:310,313,324,327`, `src/history.rs:407,431,448`, `src/time_slice.rs:565`. ## Fix `tuple_max` must propagate NaN. **`f64::max` is not the fix** — it also ignores NaN by design (`f64::max(NaN, 1.0) == 1.0`), which is the identical bug wearing a standard-library name. It needs an explicit test: ```rust fn max_propagating_nan(a: f64, b: f64) -> f64 { if a.is_nan() || b.is_nan() { f64::NAN } else if a > b { a } else { b } } ``` Once the reduction propagates, `step_is_finite` fires deterministically and the existing guards do what they document. ## Test that would have caught it A NaN reduced from a **non-final** position. Existing coverage only ever produces a NaN that is either the whole fit or the last term — precisely the case that passes. Pin `tuple_max` directly at all three orderings (NaN first, middle, last) rather than only end to end, since the end-to-end version is the one that flakes and would be a maddening intermittent failure in CI. ## Related, worth doing in the same pass `src/game.rs:301` — `while tuple_gt(step, epsilon) && iter < max_iter` is the only one of the three convergence loops with no `step_is_finite` break beside it; `history.rs:1627` and `time_slice.rs:557` both have one. Note an A/B measurement showed swapping in `!step_converged` there makes a 4-team chain run 20 000 iterations instead of 4 while producing **bit-identical** posteriors, so the fix there belongs in `Gaussian::delta` (returning `0.0` for the sigma component when both operands are improper), not in the loop condition. Found by a floating-point audit, 2026-09-09. Verified independently of the agent that reported it.

logaritmisk added the bug numerics labels 2026-09-09 14:53:50 +00:00

logaritmisk referenced this issue from a commit

2026-09-09 16:10:51 +00:00

fix!: propagate NaN through the convergence reduction

logaritmisk closed this issue

2026-09-09 16:10:51 +00:00

logaritmisk referenced this issue from a commit

2026-09-09 16:10:52 +00:00

Merge branch 'fix/numerics-critical'

Sign in to join this conversation.