-
Notifications
You must be signed in to change notification settings - Fork 2
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
fix sample encoding for export #76
Conversation
Co-authored-by: Phillip Alday <[email protected]>
# converting everything to Int16 in the actual export. | ||
samples = encode(samples) | ||
new_info = SamplesInfoV2(Tables.rowmerge(samples.info; sample_type)) | ||
return Samples(samples.data, new_info, true; validate=false) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
mildly cursed, but makes sense
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yup, hence the XXX
comment above, wanted to make sure this was known to be cursed
sample_data = vec(samples[channel_name, :].data) | ||
# manually convert here in case we have input samples whose encoded | ||
# values are convertible losslessly to Int16: | ||
sample_data = Int16.(vec(samples[channel_name, :].data)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this seems unlikely but I guess it doesn't hurt 🤷
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah, this is actually necessary when you have input samples with Int8
/UInt8
encoding (which is the "mildly cursed" bit you called out above)
I kinda think that this should be a method of |
Hm, I'm not sure about this...it's not part of the Onda documented API so I'm a bit reluctant, and it's also not really doing the same thing (IIRC, |
Co-authored-by: Phillip Alday <[email protected]>
fixes #75 and cleans up a lot of crufty old code that's gotten out of sync with both Onda and EDF.
It adds a new internal function
reencode_samples
that takesOnda.Samples
and returns new samples that are encoded in an EDF-friendly way (either directly asInt16
/the requested type, or as something that can be converted losslessly to that type). It tries very hard to not change the encoding if it's possible, based on the typemin/max of the inputsample_type
, adn failing that, the acutal min/max of the encoded values (for<:Integer
sample types). If it's not possible to losslessly represent the encoded values asInt16
, it picks a new quantization setting based on the min/max of the decoded values (fitting those to typemin/max ofInt16
or whatever teh desired type is) and returning the re-encoded samples.It also adds much more thorough tests of the export behavior, testing every type that Onda declares is allowed in
Onda.LPCM_SAMPLE_TYPE_UNION
.While adding tests, I found that it's not possible for EDF.jl to decode things where the physical min/max are anywhere near the Float32 min/max finite values, see beacon-biosignals/EDF.jl#67 As a result, some of the re-encoding tests do not hit the true typemin/max of the Onda sample type, but I think that's okay. We're still in much better shape here than we were.
I'm putting this on hold for now but want to get a draft PR up