search

beacon-biosignals / AlignedSpans.jl

Helpers for continuous <-> discrete indexing
MIT License
4 stars 0 forks source link

Support sample_time < 0 nanoseconds #15

Closed hannahilea closed 1 year ago

hannahilea commented 1 year ago 
julia> AlignedSpan(1238, TimeSpan(-32,2), RoundSpanDown)
ERROR: ArgumentError: `sample_time` must be >= 0 nanoseconds

Not sure I understand why we require times to be positive (https://github.com/beacon-biosignals/AlignedSpans.jl/blob/7f898cb7f6b0b5ecad1b9db2be3e4fc3517f8344/src/time_index_conversions.jl) but I think we shouldn't require this! Even though aligned spans are relative to sample index 1 (time 0) of some underlying signal, there's no reason they can't start before that signal does---even though actually indexing into it would cause a failure.

ericphanson commented 1 year ago

I think what happened is that these checks are from TimeSpans.index_from_time, where it does make sense to have them, and I copied them over without re-thinking. Then I did encounter this problem but I wasn't sure if it should actually be changed or not (i.e. should AlignedSpans actually represent samples that exist, always).

I agree it should be changed, because we don't e.g. store/check the duration of the recording or anything like that, so we could just as easily have spans corresponding to samples beyond the recording. Not sure why that should be allowed and not samples before the recording. I think it is better to see AlignedSpans simply as sample-rate-aligned-time-spans, not necessarily corresponding to a specific Samples object. Also, since spans can be relative and what they are relative to can change, having spans be tied to specific samples doesn't really make sense and is impractical and unhelpful. I think.