Skip to content

Reject a NaN at the RunningMedian window boundary (#284) - #354

Open
maskjelly wants to merge 1 commit into
kmolan:mainfrom
maskjelly:fix/running-median-non-finite
Open

maskjelly wants to merge 1 commit into
kmolan:mainfrom
maskjelly:fix/running-median-non-finite

Conversation

@maskjelly

Copy link
Copy Markdown

Closes #284.

RunningMedian's insertion sort compares with >, which is false in both directions against a NaN, so one NaN in the window left it unsorted and the middle slot answered with an arbitrary finite value — the one filter whose whole job is discarding a wild reading, defeated by exactly that.

Route chosen: reject at the window boundary. A NaN now never enters the window: the answer holds at the median of the samples already accepted, and the window does not age on a rejected read. Why this over partitioning the non-finite values to one end:

  • Hysteresis is the module's other filter that cannot use a NaN, and its documented behavior is exactly this — ignored, answer holds. The module table now reads the same way for both.
  • Partitioning biases the answer: on [1,2,3,4,5] a NaN would shift the window and report the upper median of the finite values (4, not 3), which is a plausible-looking number the data does not support — the class of failure this issue is about.
  • The sort's > stays a total order over everything that can be in the window, so the invariant comment can say that again.

What is deliberately unchanged: infinities are ordinary extreme readings and still enter and sort (filter_checked admits them), and filter_checked still returns SignalError::NonFinite for a NaN — it now differs from filter in reporting the rejection rather than protecting the window. The unchecked filter pays one is_nan branch; the module doc's "the per-sample calls do not look at the sample" and the tutorial's "costs no branch" claims were updated so they stay true.

One pinned choice worth a look: a window handed nothing but NaNs has never been seeded and answers 0.0 (the zero a fresh window starts from, same as a filter holding its initial state). If you would rather a dead sensor show up as NaN in that case, that is the one place this route trades "never a plausible wrong number" for "never a wrong number at all".

Tests: a window of finite readings with one NaN reports the median of the finite ones (f64 and f32), a rejected NaN leaves the window byte-identical while a subsequent +inf/-inf does take a slot, the all-NaN and all-infinity windows answer as documented. The old a_nan_moves_the_median_without_showing_it asserted the shifted answer and is replaced by these; every other test in the file is untouched. Fail-without-fix: with only the source reverted, 4 of the new tests fail (left: 2.0, right: 3.0 for the issue's exact case, left: 4.0, right: 3.0 for the boundary one); restored, the file is 20/20.

Gates: cargo fmt --all --check, cargo clippy -p multicalc --all-targets --features alloc -- -D warnings, cargo test -p multicalc, --features alloc, --all-features --doc — all green.

Not done: no change to filter_checked's error type or the other filters; consumers outside the gated crates (tools/qa, demos, multicalc-py) only feed finite samples.

`RunningMedian`'s insertion sort compares with `>`, which is false in
both directions against a NaN, so one NaN in the window left it unsorted
and the middle slot answered with an arbitrary finite value. This is the
one filter whose whole job is discarding a wild reading, and a NaN lidar
return defeated it.

A NaN is rejected at the window boundary now: it never enters the window,
so the answer holds at the median of the samples already accepted, the
window does not age on a rejected read, and the sort's `>` is left a
total order over everything that can be in it. Infinities are ordinary
extreme readings and are unchanged. `filter_checked` keeps returning
`SignalError::NonFinite` for a NaN; it differs from `filter` only in
reporting it.

The route is the one `Hysteresis` already takes for the same reason — a
NaN it cannot use is ignored and the answer holds — and it was preferred
over partitioning the non-finite values to one end, which biases the
median upward and shifts the window on a rejected sample.

A window handed nothing but NaNs has never been seeded and answers 0.0,
which the docs now state. The test that pinned the old shifted-answer
behaviour is replaced by tests for the new one; everything else in the
suite is untouched.
@maskjelly
maskjelly requested a review from kmolan as a code owner September 16, 2026 21:25

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Make RunningMedian reject a non-finite sample instead of mis-sorting it

1 participant