Skip to content
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

Support BDF files #46

Merged
merged 3 commits into from
Oct 1, 2021
Merged

Support BDF files #46

merged 3 commits into from
Oct 1, 2021

Conversation

ararslan
Copy link
Member

@ararslan ararslan commented Jun 25, 2021

BDF is BioSemi's variant of EDF that encodes samples as 24 bits each rather than 16. Here we're using BitIntegers.jl to get an Int24 type that Should™ allow us to support BDF with minimal internal changes.

Closes #33.

Current state of this PR: Works! Doesn't fully work, haven't looked too closely yet, was mostly just trying this out for fun.

@klaff
Copy link

klaff commented Jul 24, 2021

I might be missing something here. Why use Int24 as opposed to just using Int32? I don't think you save on memory due to alignment considerations (i.e. sizeof(zeros(Int24, 100)) and sizeof(zeros(Int32, 100)) both give 400) and I don't think we would be relying on things like 24-bit specific rollover characteristics.

@palday
Copy link
Member

palday commented Jul 26, 2021

@klaff I think the problem comes when moving it to/from disk: you need to interpret the blitted data as Int24 and also blit it back out as Int24. Once it's in memory though .....

@klaff
Copy link

klaff commented Jul 26, 2021

Ah, so some bit-twiddling like:

function convert_24b_bytes_to_Int32(MSB, midB, LSB)
    return reinterpret(Int32, [0x00, LSB, midB, MSB])[1] >> 8
end

@ararslan
Copy link
Member Author

ararslan commented Jul 27, 2021

I think we've generally tried to keep the internal representations of things as close as possible to how the format stores them, for better or for worse. (One notable deviation is that the array of samples for each signal is one big vector and is not divided into data records. Annotations, however, are divided into data records.) Keeping the encoded representation true to the format also simplifies both the reading and writing; no extra conversions or checks are required. For actually using the data, you'd want to call EDF.decode anyway, which will give you a Float32 array for both EDF and BDF files.

@ararslan

This comment has been minimized.

BDF is BioSemi's variant of EDF that encodes samples as 24 bits each
rather than 16. Here we're using BitIntegers.jl to get an `Int24` type
that allows us to support BDF with minimal internal changes.
@ararslan ararslan marked this pull request as ready for review September 30, 2021 00:15
@@ -47,7 +47,6 @@ end

function edf_write(io::IO, value, byte_limit::Integer)
edf_value = _edf_repr(value)
@assert isascii(edf_value)
Copy link
Member Author

Choose a reason for hiding this comment

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

Note that this requirement had to be removed because the version field of a BDF header begins with a non-ASCII byte.

@ararslan ararslan changed the title WIP: Support BDF files Support BDF files Sep 30, 2021
@ararslan ararslan requested review from klaff and palday September 30, 2021 00:20
Project.toml Outdated Show resolved Hide resolved
src/read.jl Outdated Show resolved Hide resolved
@ararslan ararslan requested a review from palday October 1, 2021 18:20
@ararslan ararslan merged commit eaa69a9 into master Oct 1, 2021
@ararslan ararslan deleted the aa/bdf branch October 1, 2021 23:13
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.

Support BDF/BDF+
3 participants