Skip to content

variance_measure_add: skip non-finite input instead of asserting #351

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
rocketmark wants to merge 1 commit into
base: master
Choose a base branch
from
Open

variance_measure_add: skip non-finite input instead of asserting #351

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 12 additions & 1 deletion redist/variance.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,10 +12,21 @@ static inline void variance_measure_add(struct variance_measure *meas, const FLT
if (meas->size == 0)
meas->size = 1;

/* Guard against non-finite values. Corrupt optical angles (e.g. bad FPGA
* timestamps during USB disturbances) can reach here as NaN or Inf. The
* assert below would crash the process and corrupt the on-disk config;
* dropping one sample is strictly safer and has negligible effect on the
* variance estimate. */
for (int i = 0; i < meas->size; i++) {
if (!isfinite(d[i])) {
fprintf(stderr, "[libsurvive] variance_measure_add: non-finite d[%d]=%f, dropping measurement\n", i, (double)d[i]);
return;
Copy link
Copy Markdown
Collaborator

@bl4ckb0ne bl4ckb0ne Mar 13, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure if all of the other measures should be thrown out when only one of them is finite.

Copy link
Copy Markdown
Contributor Author

@rocketmark rocketmark Mar 13, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The shared n counter makes partial updates inconsistent. variance_measure_calc computes:

d[i] = (sumSq[i] - (sum[i] * sum[i]) / n) / n

n is a single value for all dimensions. If we skip the NaN dimension but still increment n, the variance for that dimension uses the wrong denominator (too large). If we update the finite dimensions but don't increment n, those dimensions' variance calculations are also wrong (too small).

A complete solution per-dimension would be per-dimension n counters, which would be a larger struct change outside the scope of this fix.

In practice, when a USB disturbance produces a NaN angle, the other angles in the same d[] bundle are from the same corrupted sensor read and are likely also unreliable. Dropping the whole sample feels like the safer choice here.

If you'd prefer, I can change the guard to only trigger if all elements are non-finite, but that would let through mixed good/NaN samples that would silently corrupt the variance for the NaN dimension. Happy to discuss which behaviour you'd prefer.

}
}

meas->n++;
addnd(meas->sum, meas->sum, d, meas->size);
for (int i = 0; i < meas->size; i++) {
assert(isfinite(d[i]));
meas->sumSq[i] += d[i] * d[i];
}
}
Expand Down
Loading