-
Notifications
You must be signed in to change notification settings - Fork 308
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.
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
Record.adc: RuntimeWarning: invalid value encountered in cast #480
Comments
Never mind: |
tompollard
added a commit
that referenced
this issue
Apr 19, 2024
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`.
To reproduce the issue:
or
This is fixed with pull #481. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
The
adc
function reports a RuntimeWarning (at least in some cases) if the input array contains NaN values.I'm currently looking at this case (expanded=True, inplace=False), though it looks like the other cases are also broken in various similar ways:
The warning occurs here:
numpy is complaining, I assume, because you can't cast a NaN to an integer.
(Like many things to do with numpy and with this package), the above looks rather inefficient. Perhaps
numpy.nan_to_num
could be used instead.The text was updated successfully, but these errors were encountered: