Skip to content

Handle all-NaN channels in calc_adc_params (fixes #485)#571

Open
Leonard013 wants to merge 1 commit into
MIT-LCP:mainfrom
Leonard013:fix/485-all-nan-channel
Open

Handle all-NaN channels in calc_adc_params (fixes #485)#571
Leonard013 wants to merge 1 commit into
MIT-LCP:mainfrom
Leonard013:fix/485-all-nan-channel

Conversation

@Leonard013

Copy link
Copy Markdown

Fixes #485.

Why

Writing a signal whose channel is entirely NaN crashes:

import numpy as np, wfdb
wfdb.wrsamp('xxx', fs=500, units=['mV'], sig_name=['I'],
            p_signal=np.array([[np.nan]]), fmt=['16'])
# ValueError: cannot convert float NaN to integer

Two root causes in wfdb/io/_signal.py, both noted in #485:

  1. The all-NaN guard never runs. calc_adc_gain_baseline has
    if pmin == np.nan: which is always False (NaN == NaN is False). For an
    all-NaN channel pmin/pmax are NaN, so execution falls through to the
    "regular varied signal" branch, computes a NaN baseline, and dies at
    baseline = int(np.floor(baseline)) with ValueError: cannot convert float NaN to integer.
  2. Unsuppressed warning. calc_adc_params calls np.nanmin/np.nanmax on
    the all-NaN column, emitting RuntimeWarning: All-NaN slice encountered. The
    existing # Should suppress warning message. comment shows this was already
    intended to be silenced.

What

  • calc_adc_gain_baseline: if pmin == np.nan:if np.isnan(pmin):, so the
    pre-existing all-NaN branch (which sets adc_gain = 1, baseline = 1) actually
    executes.
  • calc_adc_params: wrap the np.nanmin/np.nanmax calls in
    warnings.catch_warnings() + simplefilter("ignore", RuntimeWarning), scoped to
    just those two lines.
  • Added import warnings.

Note on suppression method

np.errstate(...) does not silence this warning — "All-NaN slice encountered"
is raised through Python's warnings machinery (a RuntimeWarning), not the
floating-point error state. warnings.catch_warnings() is required. Verified
empirically.

Why the sentinel gain/baseline is correct (NaN → invalid-value round-trip)

WFDB stores a missing/invalid physical sample as the format's reserved digital
sentinel INVALID_SAMPLE_VALUE[fmt] (e.g. -2**15 = -32768 for fmt 16), the
lowest digital code — which is exactly why calc_adc_gain_baseline does
dmin = dmin + 1 (reserve the bottom code for NaN).

In adc (adc_inplace), NaN positions are recorded up front and, after the
gain/baseline multiply-add, overwritten with the sentinel
(np.copyto(p_signal, d_nan, where=nanlocs)). For an all-NaN channel every sample
is a nanloc, so gain/baseline are never reflected in the stored data — they only
need to be a finite gain and an in-range int baseline so the .hea is well-formed.
On read, dac maps d_signal == d_nan back to np.nan before applying
gain/baseline. Hence the round-trip is exact regardless of the sentinel
gain/baseline, and this is identical to how a partial-NaN channel's individual
NaN samples are already handled (test_physical_conversion: p_signal[5:10]=nan
d_signal[5:10]=-32768).

Observed round-trip (fmt 16): all-NaN channel → header adc_gain=1.0, baseline=1,
every digital sample -32768, read back as all-NaN. Mixed 2-channel signal: normal
channel round-trips to max abs err 0.0; all-NaN channel reads back all-NaN. No
warnings.

Tests

Added to tests/test_record.py::TestSignal, following the existing
test_adc_gain_{max,min}_boundary write/read round-trip style:

  • test_all_nan_single_channel — single all-NaN channel; asserts no
    RuntimeWarning on write and read-back is all-NaN.
  • test_all_nan_mixed_channels — 2 channels, one all-NaN + one normal; asserts no
    RuntimeWarning, normal channel round-trips within quantization tolerance, NaN
    channel reads back all-NaN.

Both fail before the fix (ValueError: cannot convert float NaN to integer)
and pass after. Full tests/test_record.py, tests/test_multi_record.py,
tests/test_processing.py pass. black --check clean.

Notes for reviewers

  • adc_gain=1, baseline=1 defaults are the pre-existing all-NaN branch values
    (the fix just makes the branch reachable). Any finite gain / in-range integer
    baseline round-trips correctly since the samples are all the reserved sentinel;
    happy to switch to baseline = 0 if preferred.
  • Scope: the same np.nanmin/np.nanmax warning could arise in the
    e_p_signal (expanded) branch of calc_adc_params. The crash is already fixed
    there (that branch shares calc_adc_gain_baseline); warning suppression was scoped
    to the documented p_signal path (the one with the # Should suppress comment,
    exercised by Handle all-NaN channels in calc_adc_params #485). Can extend to the expanded path if wanted.
  • docs/changes.rst is batched by maintainers at release time (current 4.3.1 has
    no unreleased section), so no changelog entry is included.

Prepared with AI assistance (Claude Code): the reproduction, root-cause analysis,
fix, and tests were AI-drafted; the reasoning and verification (including the
NaN→sentinel round-trip trace) are documented above.

Writing a channel that is entirely NaN raised
"ValueError: cannot convert float NaN to integer" and emitted an
unsuppressed "All-NaN slice encountered" RuntimeWarning.

- calc_adc_gain_baseline: `if pmin == np.nan` is always False, so the
  intended all-NaN branch never ran and NaN reached int(np.floor(...)).
  Use np.isnan(pmin) so the branch (adc_gain=1, baseline=1) executes.
- calc_adc_params: scope np.nanmin/np.nanmax in warnings.catch_warnings()
  to silence the all-NaN RuntimeWarning (honors the existing
  "Should suppress" comment). np.errstate does not suppress this warning.

Every NaN sample already maps to the format's reserved invalid-sample
value on write and back to NaN on read, so an all-NaN channel round-trips
to all-NaN, consistent with partial-NaN handling.

Adds regression tests test_all_nan_single_channel and
test_all_nan_mixed_channels.

Fixes MIT-LCP#485.

This change was written with the assistance of Claude (Anthropic).

Co-authored-by: Claude <noreply@anthropic.com>
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.

Handle all-NaN channels in calc_adc_params

1 participant