Allow converting encoded samples if the element type matches (#171)

At present, we error on `convert(::Type{Samples{A}}, ::Samples{B})` for
all `A` and `B` if the samples are encoded. This is reasonable since we
don't want to affect how the samples get decoded. However, decoding
should be based on the type of the encoded samples themselves rather
than of their container; a decoder shouldn't care whether the incoming
data is e.g. a full matrix or a view of a matrix. Thus we can allow
conversion for encoded samples provided that only the container type is
affected, not the element type.

Project.toml

@@ -1,7 +1,7 @@
 name = "Onda"
 uuid = "e853f5be-6863-11e9-128d-476edb89bfb5"
 authors = ["Beacon Biosignals, Inc."]
-version = "0.15.8"
+version = "0.15.9"
 
 [deps]
 Arrow = "69666777-d1a9-59fb-9406-91d4454c9d45"

src/samples.jl

@@ -81,7 +81,11 @@ function Base.hash(a::Samples, h::UInt)
 end
 
 function Base.convert(::Type{Samples{T}}, samples::Samples) where {T}
-    samples.encoded && throw(ArgumentError("can't `convert` encoded samples; use `decode` first"))
+    # For encoded samples, allow converting between container types so long as the element
+    # type remains the same
+    if samples.encoded && eltype(T) != eltype(samples.data)
+        throw(ArgumentError("can't `convert` encoded samples to a different element type; use `decode` first"))
+    end
     return Samples(convert(T, samples.data), samples.info, samples.encoded)
 end
 

test/samples.jl

@@ -232,19 +232,29 @@ end
 
     # In particular, this fixes an arrow deserialization issue
     # (https://github.com/beacon-biosignals/Onda.jl/issues/156)
-    table = [(; col = samples), (; col = samples)]
+    table = [(; col=samples), (; col=samples)]
     # Here, materializing this table in this way threw an error before `convert` was defined
     rt_table = DataFrame(Arrow.Table(Arrow.tobuffer(table)); copycols=true)
     rt = rt_table[1, "col"]
     @test rt.data isa AbstractMatrix{Float32}
     @test rt == samples
 
-    # For encoded samples, in generally we cannot `convert`.
+    # For encoded samples, in general we cannot `convert` between element types, as this
+    # could affect downstream decoding.
     # We choose to not update encoding parameter in `convert`, since `convert` can be implied
     # implicitly, and changing the encoding parameters seems like too big of a change.
     samples = Samples(rand(sample_type(info), 3, 100), info, true)
-    err = ArgumentError("can't `convert` encoded samples; use `decode` first")
+    err = ArgumentError("can't `convert` encoded samples to a different element type; use `decode` first")
     @test_throws err convert(Samples{Matrix{Int32}}, samples)
+
+    # If the element type remains the same, we can `convert` between container types, as
+    # this is unlikely to affect decoding (unless a truly deranged method has been defined)
+    T = sample_type(info)
+    samples = Samples(view(rand(T, 3, 100), :, :), info, true)
+    @test samples.data isa SubArray{T}
+    s2 = convert(Samples{Matrix{T}}, samples)
+    @test s2.data isa Matrix{T}
+    @test samples == s2
 end
 
 @testset "Base.copy" begin