Expand byte size of samples_per_record
#92
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #92 +/- ##
=======================================
Coverage 95.25% 95.25%
=======================================
Files 4 4
Lines 295 295
=======================================
Hits 281 281
Misses 14 14 ☔ View full report in Codecov by Sentry. |
haberdashPI
force-pushed
the
dfl/exapnd-samples-per-record
branch
from
c4aab2b to
06b47a6
Compare
haberdashPI
requested review from
ararslan,
patinoam and
kleinschmidt
and removed request for
ararslan
Project.toml
Outdated
| @@ -1,7 +1,7 @@ | |||
| name = "EDF" | |||
| uuid = "ccffbfc1-f56e-50fb-a33b-53d1781b2825" | |||
| authors = ["Beacon Biosignals, Inc."] | |||
| version = "0.7.5" | |||
| version = "0.7.6" | |||
There was a problem hiding this comment.
| version = "0.7.6" | |
| version = "0.8.0" |
src/types.jl
Outdated
| end | ||
|
|
||
| function SignalHeader(signal::AnnotationsSignal) | ||
| return SignalHeader("EDF Annotations", "", "", -1, 1, -32768, 32767, | ||
| return SignalHeader("EDF Annotations", "", "", -1, 1, -32768, 32767, # typemin/typemax |
There was a problem hiding this comment.
I'm not sure the typemin/typemax comment is actually that informative; it may even be more confusing since the field is now typed as Int32 but those values are not typemin(Int32)/typemax(Int32), they're for Int16, which will be otherwise irrelevant after this PR.
There was a problem hiding this comment.
this field is still Int16 I think (it's the digital min/max)
There was a problem hiding this comment.
If it's creating any confusion at all, I'll just remove it.
There was a problem hiding this comment.
Perhaps this is only helpful for my weird brain 😅
There was a problem hiding this comment.
Ha, I think my confusion only arose because of the context of the change (I assumed without checking that it was related to the field that's having its type widened)
readme revisions Co-authored-by: Alex Arslan <[email protected]>
There was a problem hiding this comment.
Approved on my end - all changes from Int16 to Int32 are reasonable! However you should probably hold off on merging until @kleinschmidt / @ararslan / @ericphanson approve
This upgrades OndaEDF.jl to EDF.jl 0.8 (c.f. beacon-biosignals/EDF.jl#92), which expands the field `sample_per_record` from `Int16` to `Int32`, mandating the creation of a new `PlanV3` and `FilePlanV3` that can accommodate this wider type. Co-authored-by: Phillip Alday <[email protected]>
samples_per_recordis stored as anInt16which can cause errors when individual records of an EDF are very large. This expands the size toInt32.