Skip to content

Handle all-NaN channels in calc_adc_params - #628

Merged
tompollard merged 1 commit into
MIT-LCP:mainfrom
RClaudiuM:fix/485-adc-all-nan-channel
Nov 8, 2026
Merged

tompollard merged 1 commit into
MIT-LCP:mainfrom
RClaudiuM:fix/485-adc-all-nan-channel

Conversation

@RClaudiuM

Copy link
Copy Markdown
Contributor

Fixes #485.

Writing a record where one channel is entirely NaN fails:

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

Both causes are the ones described in the issue:

  • calc_adc_gain_baseline checked pmin == np.nan, which is always False, so the all-NaN branch never ran. It now uses np.isnan(pmin).
  • calc_adc_params used np.nanmin/np.nanmax, which warn on all-NaN channels. It now uses np.fmin.reduce/np.fmax.reduce (as suggested in the issue), which give the same result without the warning, for both p_signal and e_p_signal.

An all-NaN channel now gets adc_gain=1, baseline=1 (the existing intent of that branch), is written as the NaN sample value, and reads back as NaN.

Testing:

  • New TestSignal.test_all_nan_channel (runs with RuntimeWarning as an error): checks calc_adc_params for p_signal and e_p_signal, then a wrsamp/rdrecord round trip with one all-NaN channel and one normal channel. It fails on main and passes with this change.
  • uv run pytest -m "not network": 335 passed.
  • ruff format --check, ruff check and towncrier build --draft pass. Changelog entry added in changelog.d/.

The all-NaN guard in calc_adc_gain_baseline compared pmin == np.nan,
which is always False, so a channel containing only NaN values fell
through to the regular case and failed with "cannot convert float NaN
to integer". Use np.isnan instead.

Compute the channel minimum and maximum with np.fmin/np.fmax.reduce,
which ignore NaN like nanmin/nanmax but do not warn about all-NaN
channels.

@tompollard tompollard left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks for this, and for the clear write-up. The pmin == np.nan check could never be true, so it's good to have that fixed. Switching to np.fmin.reduce / np.fmax.reduce as suggested in the issue removes the warnings neatly.

I checked out the branch and everything passes for me: the offline suite, ruff and your new test, which fails on main as expected. I also tried a few other inputs, such as an all-NaN float32 channel and an all-NaN channel passed as a plain list to e_p_signal, and they work too. Running the test with RuntimeWarning as an error is a nice touch.

Happy to merge!

@tompollard
tompollard merged commit cc9f94f into MIT-LCP:main Nov 8, 2026
18 checks passed
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

2 participants