search

MIT-LCP / wfdb-python

Native Python WFDB package
MIT License
730 stars 298 forks source link

Fix NaN handling in Record.adc, and other fixes #481

Closed bemoody closed 3 months ago

bemoody commented 3 months ago

Fix several bugs in Record.adc:

  1. Previously, the function would try to convert all samples to integers and then, for any samples that were NaN, replace the corresponding elements with the appropriate sentinel value. Even though this was probably safe in most cases, casting NaN to an integer is implementation-defined behavior, and raises a warning by default (issue #480).

  2. NaN just plain wasn't handled for the inplace=True, expanded=False case. (Currently, we don't use inplace=True anywhere internally; although it saves a bit of memory, it's destructive and so it's probably wise for high-level functions like wrsamp to avoid it.)

  3. The expanded=True case relied on self.n_sig (in contrast to expanded=False, which operates based on the dimensions of p_signal.) This meant it would fail if the caller didn't explicitly set n_sig, which was an annoying inconsistency.

Also, tidy up duplicated code and make things a little more efficient.

A side note: I don't think the inplace=True mode is particularly great to have. It conflates two things (modifying the Record object attributes, which many applications want; and modifying the array contents, which you may think you want until you realize it subtly breaks something.) It does save some memory, but not as much as you'd hope. (That copy=False is pretty much a lie.) And of course I don't like functions whose return type is dependent on their arguments. So I would definitely put inplace on the chopping block for 5.0.0. Still, I think the updated code here isn't too terribly ugly.

This set of changes is the first step to making wfdb.wrsamp work for multi-frequency (issue #336). Next is to fix Record.calc_adc_params, then Record.set_d_features.

tompollard commented 3 months ago

@bemoody this looks good to me. were the tests intentionally stopped?

tompollard commented 3 months ago

Hmm, it looks like maybe it is failing on format checks (https://github.com/MIT-LCP/wfdb-python/actions/runs/8744000168/job/24038235919) and then the remaining tests are being cancelled.

bemoody commented 3 months ago

Yeah, I think it should work, it's failing because the latest version of black requires formatting changes (not related to this pull AFAIK.)

tompollard commented 3 months ago

Yeah, I think it should work, it's failing because the latest version of black requires formatting changes (not related to this pull AFAIK.)

I assume these issues were fixed in https://github.com/MIT-LCP/wfdb-python/pull/482? If so, please could you rebase this PR on main?