Conversation
`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.
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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:
Hysteresisis 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.[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.>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_checkedadmits them), andfilter_checkedstill returnsSignalError::NonFinitefor a NaN — it now differs fromfilterin reporting the rejection rather than protecting the window. The uncheckedfilterpays oneis_nanbranch; 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 (
f64andf32), a rejected NaN leaves the window byte-identical while a subsequent+inf/-infdoes take a slot, the all-NaN and all-infinity windows answer as documented. The olda_nan_moves_the_median_without_showing_itasserted 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.0for the issue's exact case,left: 4.0, right: 3.0for 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.