Separate proper signals from annotations signals

[ararslan]

Currently EDF.File stores EDF.Signals and EDF.AnnotationsSignals 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 #29. However, this leads to some common and annoying patterns like branching on the type during iteration. One immediate example is

EDF.jl/src/read.jl

Lines 195 to 201 in 87e6823

	total_expected_samples = sum(signals) do signal
	if signal isa Signal
	return signal.header.samples_per_record
	else
	return signal.samples_per_record
	end
	end

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:

diff --git a/src/types.jl b/src/types.jl
index 30150d8..36800db 100644
--- a/src/types.jl
+++ b/src/types.jl
@@ -251,12 +251,14 @@ Type representing an EDF file.
 
 * `io::I`
 * `header::FileHeader`
-* `signals::Vector{Union{Signal,AnnotationsSignal}}`
+* `signals::Vector{Signal}`
+* `annotations::Vector{AnnotationsSignal}`
 """
 struct File{I<:IO}
     io::I
     header::FileHeader
-    signals::Vector{Union{Signal,AnnotationsSignal}}
+    signals::Vector{Signal}
+    annotations::Vector{AnnotationsSignal}
 end
 
 function Base.show(io::IO, edf::File)

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:

• We lose the order in which the data and annotations signals were intermixed in the file. This can be maintained with a bit of extra tracking but that in turn introduces weird complexity.
• Iteration over all of the signals in the file, both data and annotations, would require two loops instead of one. For example, the linked code above would become something like

  total_expected_samples = sum(s -> s.header.samples_per_record, file.signals) +
                           sum(a -> a.samples_per_record, file.annotations)

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.

[palday]

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.

[ararslan]

cc also @jrevels, who introduced the current structure

[jrevels]

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.

[ararslan]

Yeah, that works for me.