beacon-biosignals / Onda.jl

A Julia package for high-throughput manipulation of structured signal data across arbitrary domain-specific encodings, file formats and storage layers
Other
68 stars 5 forks source link

validate Samples duration matches Signal duration upon `store!` #33

Closed SimonDanisch closed 4 years ago

SimonDanisch commented 4 years ago

It seems one is able to save an Onda dataset, with less samples saved then described by start/stop time & sample_rate. E.g, this will run through fine, even though I just have half half the samples from what I promise:

using Onda, Dates

path = "test.onda"
dset = Onda.Dataset(path, create=!isdir(path))
uuid, recording = Onda.create_recording!(dset)

function test_samples(sample_rate_hz, duration_s)
    nsamples = round(Int, duration_s * sample_rate_hz)
    channels = [:spikes, :threshold_spikes]
    signal = Signal(channels, Nanosecond(0), Nanosecond(duration_s * 1e9), :probability, 0.01, 0.0, Int16, sample_rate_hz, Symbol("lpcm.zst"), nothing)
    # Divide by 2, for wrong nsamples amount!
    data = zeros(length(channels), nsamples ÷ 2)
    samples = Samples(signal, false, data)
    return samples
end

sample = test_samples(10, 10*60)

store!(dset, uuid, :test, sample, overwrite=true)
Onda.save_recordings_file(dset)

Loading also seems fine:

dset2 = Onda.Dataset(path)
test = load(dset2, uuid, :test)

But indexing errors:

test[1, Nanosecond((10 * 60 * 1e9)-1)]

Note, that there is also an off by one problem (Or am i misunderstanding something? But I should be able to index with duration(samples), to get the last sample, no?

It seems mandatory to validate this in the Samples constructor, so we don't need to do this kind of error checking in e.g. OndaViz and avoid, that people save out lots of datasets with missing samples, just to notice it months later ;) cc @anoojpatel

jrevels commented 4 years ago

ref https://github.com/beacon-biosignals/Onda.jl/issues/30#issue-577144938, we should use the same strategy for this.

I'll cook up a PR.

jrevels commented 4 years ago

It seems mandatory to validate this in the Samples constructor

should be noted (a la #34) that this specific thing needs to be validated on store!, but is not relevant to every Samples construction given that data length need not match the full length of the recording's signal in context (e.g. if it's only a slice)

jrevels commented 4 years ago

closed by #35