Closed kleinschmidt closed 1 year ago
As far as I can tell, we've never really tested for export with UInt sample types, despite the fact that these have been supported by Onda at least since 0.11...https://github.com/beacon-biosignals/Onda.jl/blob/8b97a05e12be68a95d6069be2d25d6db2db1a913/src/samples.jl#L162C89-L162C89
The rabbit hole goes deeper: the resolution scaling that's applied to any sample type wider than 16 bits is wrong. It makes the resolution bigger by a factor of sizeof(sample_type) / sizeof(Int16)
, which for Int32
is 2x. But the number of representable values is actually different by a factor of 2^16
(exponential scaling, not linear, in the number of bits/bytes).
So, what this means is that exporting signals with extreme values that use wider encoding will just silently clip those values.
I think using things like Int32
for encoding is pretty rare in practice, but I do think we should try to handle this more correctly. I'm not really sure what the right thing to do is here though...look at the actual values to choose the resolution? just annihilate the resolution by >1000x and maybe warn the user?
...the resolution scaling that's applied to any sample type wider than 16 bits is wrong....
Does this meant this should actually just be Int16
? Currently, we are accepting Int16
, Int32
, and Int64
as is
It's wrong only inasmuch as extreme values won't be properly represented; you'd need to scale resolution by a factor of 256^2 instead of 2. But we're talking really extreme values here; in most cases, it will be fine (and the extreme values will be clipped). However if you have a signal with a tiiiiny resolution that's using all the dynamic range of the Int32 or Int64 type, then you could get into trouble.
MWE:
From digging around a bit in export, I think I have a hunch about what happened here, which is that when EDF added support for BDF (which use Int24 storage), the signal data field of hte
EDF.Signal
struct was made parametric on the eltype, and the implicitconvert(Vector{Int16}, data)
behavior in the old constructor we were relying on no longer happens.