beacon-biosignals / TimeSpans.jl

A Julia package that provides a `TimeSpan` type for representing a continuous span between two points in time.
Other
6 stars 2 forks source link

Suspected implementation bug in `index_from_time(sample_rate, span::TimeSpan)` #28

Closed ericphanson closed 2 years ago

ericphanson commented 2 years ago

The implementation of the semantics isn't correct when the right endpoint of the span doesn't fall exactly on a sample; we always throw away the right-endpoint even when it is well within the span:

https://github.com/beacon-biosignals/TimeSpans.jl/blob/eb8b7949b3bc4b82c7f176f3b5227a48c08f4dce/src/TimeSpans.jl#L237

(unless that would make an empty interval, in which case we keep it).

@kleinschmidt's suggestion

I think the solution might be to convert the final index back to time, and see if it needs to be adjusted down by 1 (e.g., if time_from_index(rate, index_from_time(rate, stop(span))) == stop(span) then you use j-1, otherwise use j

would fix that, I think.

Originally posted by @ericphanson in https://github.com/beacon-biosignals/TimeSpans.jl/issues/26#issuecomment-1017890255