ConstantDrift(-x) is bit-identical to ConstantDrift(x) — seal the field #65

New Issue

2026-09-09T15:51:36Z

logaritmisk commented

2026-09-09 15:51:36 +00:00

Split from #61, which fixed the rest of this defect class. This is the one entry point left, and it needs a breaking API change rather than a validation line.

The defect

ConstantDrift's gamma enters only as gamma * gamma:

fn variance_for_elapsed(&self, elapsed: i64) -> f64 {
    elapsed.max(0) as f64 * self.0 * self.0
}

so the sign is squared away. Measured, two competitors over two slices:

ConstantDrift( 0.0833) -> pi = 0.022968176413876842, tau = 0.701122785949954
ConstantDrift(-0.0833) -> pi = 0.022968176413876842, tau = 0.701122785949954

Bit-identical. The sign is neither rejected nor honoured; it vanishes.

This is exactly what HistoryBuilder::sigma, HistoryBuilder::beta,
Gaussian::from_ms, Rating::new and Member::with_drift_scale all now
reject. ConstantDrift is the only one left.

Why #61 could not close it

ConstantDrift is a tuple struct with a public field:

pub struct ConstantDrift(pub f64);

There is no constructor to intercept. Two things follow:

Validation would have to live in variance_for_elapsed, which runs inside the
sweep — so a construction-time mistake would panic mid-inference, and #61 has
a worked example of why that is the wrong place for a guard (rejecting NaN
sigma in from_ms turned the NonFiniteResult reporting path into a crash).
Checking the variance cannot see it. converge now validates the drift
variance each competitor accumulates, which catches a NaN or infinite gamma —
but gamma^2 for a negative gamma is a perfectly good variance, so that check
passes and must.

Worth being clear that the resulting model is valid: ConstantDrift(-0.05) is
the same model as ConstantDrift(0.05). Nothing is computed wrongly. The defect
is that a caller who typed a minus sign meant something, and got no indication
that it was discarded.

The fix, and its cost

Seal the field and validate in a constructor:

pub struct ConstantDrift(f64);

impl ConstantDrift {
    /// # Panics
    /// Panics unless `gamma` is finite and non-negative.
    pub fn new(gamma: f64) -> Self { /* assert */ }

    #[must_use]
    pub fn gamma(&self) -> f64 { self.0 }
}

The cost is real and is why it was deferred: ConstantDrift(x) is positional
and appears in essentially every test, every doc example, the README, the
benchmarks and both consumers. All of them become ConstantDrift::new(x).

Options, roughly increasing in disruption:

Seal it, as above. One mechanical rename across the tree, and every future
caller is checked. Breaking.
Keep the tuple struct, add new, and document ConstantDrift(x) as the
unchecked form. Non-breaking, and pointless — the unchecked path stays and is
the shorter one, so it is what people will keep writing.
Debug-assert in variance_for_elapsed. Free, and worthless in release,
which is the configuration this crate has repeatedly found defects hiding in.

(1) is the only one that actually closes it. Worth pairing with the next
breaking release rather than cutting one for it alone.

While here

A test that enumerates every public constructor and asserts each rejects
negative, NaN and infinite parameters would be worth more than the fix itself. I
declared this boundary complete twice in one session and was wrong both times —
first missing Game entirely, then missing the constructors beneath it — because
each time I validated the layer I had just touched and inferred the rest. An
enumerating test is the thing that would have caught all three rounds at once.

Found by a floating-point audit, 2026-09-09. Deferred from #61 with the sign
behaviour documented on ConstantDrift in the meantime.

Split from #61, which fixed the rest of this defect class. This is the one entry point left, and it needs a breaking API change rather than a validation line. ## The defect `ConstantDrift`'s `gamma` enters only as `gamma * gamma`: ```rust fn variance_for_elapsed(&self, elapsed: i64) -> f64 { elapsed.max(0) as f64 * self.0 * self.0 } ``` so the sign is squared away. Measured, two competitors over two slices: ``` ConstantDrift( 0.0833) -> pi = 0.022968176413876842, tau = 0.701122785949954 ConstantDrift(-0.0833) -> pi = 0.022968176413876842, tau = 0.701122785949954 ``` Bit-identical. The sign is neither rejected nor honoured; it vanishes. This is exactly what `HistoryBuilder::sigma`, `HistoryBuilder::beta`, `Gaussian::from_ms`, `Rating::new` and `Member::with_drift_scale` all now reject. `ConstantDrift` is the only one left. ## Why #61 could not close it `ConstantDrift` is a tuple struct with a public field: ```rust pub struct ConstantDrift(pub f64); ``` There is no constructor to intercept. Two things follow: - Validation would have to live in `variance_for_elapsed`, which runs inside the sweep — so a construction-time mistake would panic mid-inference, and #61 has a worked example of why that is the wrong place for a guard (rejecting NaN sigma in `from_ms` turned the `NonFiniteResult` reporting path into a crash). - Checking the *variance* cannot see it. `converge` now validates the drift variance each competitor accumulates, which catches a NaN or infinite gamma — but `gamma^2` for a negative gamma is a perfectly good variance, so that check passes and must. Worth being clear that the resulting model is *valid*: `ConstantDrift(-0.05)` is the same model as `ConstantDrift(0.05)`. Nothing is computed wrongly. The defect is that a caller who typed a minus sign meant something, and got no indication that it was discarded. ## The fix, and its cost Seal the field and validate in a constructor: ```rust pub struct ConstantDrift(f64); impl ConstantDrift { /// # Panics /// Panics unless `gamma` is finite and non-negative. pub fn new(gamma: f64) -> Self { /* assert */ } #[must_use] pub fn gamma(&self) -> f64 { self.0 } } ``` The cost is real and is why it was deferred: `ConstantDrift(x)` is positional and appears in essentially every test, every doc example, the README, the benchmarks and both consumers. All of them become `ConstantDrift::new(x)`. Options, roughly increasing in disruption: 1. **Seal it**, as above. One mechanical rename across the tree, and every future caller is checked. Breaking. 2. **Keep the tuple struct, add `new`**, and document `ConstantDrift(x)` as the unchecked form. Non-breaking, and pointless — the unchecked path stays and is the shorter one, so it is what people will keep writing. 3. **Debug-assert in `variance_for_elapsed`.** Free, and worthless in release, which is the configuration this crate has repeatedly found defects hiding in. (1) is the only one that actually closes it. Worth pairing with the next breaking release rather than cutting one for it alone. ## While here A test that enumerates **every** public constructor and asserts each rejects negative, NaN and infinite parameters would be worth more than the fix itself. I declared this boundary complete twice in one session and was wrong both times — first missing `Game` entirely, then missing the constructors beneath it — because each time I validated the layer I had just touched and inferred the rest. An enumerating test is the thing that would have caught all three rounds at once. Found by a floating-point audit, 2026-09-09. Deferred from #61 with the sign behaviour documented on `ConstantDrift` in the meantime.