Open ararslan opened 3 years ago
I like this. Branching on type seems distinctly unjulian and we're still preserving the logical structure and data equivalence of the file, even if a round-tripped file won't be bit-for-bit identical.
But I would do this after the BDF support is added. Then anybody who doesn't like this design decision and wants to stick with the older version still has good EDF/BDF support.
cc also @jrevels, who introduced the current structure
My bias for decisions like these is to preserve the underlying structure implied by the format specification and provide convenience functions on top as necessary, rather than to adopt a different underlying structure for the sake of perceived convenience. That actually was the motivation behind #29.
Why not just provide a convenience function (or two) that returns the iterators you'd like? It'd be a less disruptive change, seems to solve the problems you mentioned, and avoids the downsides you mention in the OP.
Yeah, that works for me.
Currently
EDF.File
storesEDF.Signal
s andEDF.AnnotationsSignal
s together in a single vector, closely mimicking how the file itself stores signals, and allowing us to store the signals in the order in which they appear in the file. This structure was introduced in https://github.com/beacon-biosignals/EDF.jl/pull/29. However, this leads to some common and annoying patterns like branching on the type during iteration. One immediate example is https://github.com/beacon-biosignals/EDF.jl/blob/87e68230d8bb1695094f33b073b641aeb2c016ed/src/read.jl#L195-L201 I find myself working around this structure often in similar ways.I would propose things be restructured somewhere between the current state and the structure prior to #29:
Storing the annotations as a vector permits one of the cases fixed in #29: multiple "EDF Annotations" signals in a single file.
The primary downsides to this proposition:
I personally think this mismatch between the on-disk and Julia representations (one of only two mismatches I can think of offhand) is worth it for the ergonomics but I'd be interest to hear what others think.