search

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

improve consistency of time <-> index conversion #51

Closed jrevels closed 1 year ago

jrevels commented 1 year ago

I'm working on upgrading Onda.jl and in the process trying to tackle https://github.com/beacon-biosignals/Onda.jl/issues/123, but running into problems because of behaviors similar to this:

julia> index_from_time(1.5, time_from_index(1.5, 1:5))
1:6

julia> index_from_time(1.5, time_from_index(1.5, 1:6))
1:6

this PR basically is an attempt to make a more holistic/consistent fix for the kind of bug that #29 was trying to fix, namely incorrect values due to the erroneous inclusion/consideration of the span's "excluded" upper bound

similarly makes time_from_index more consistent

I don't view this as a breaking change as much as a continuation of the bug fix in #29 , but if folks want to see a bump to v0.4 here I can do so

jrevels commented 1 year ago

However I thought I understood the code on main, and I don't really understand this. Would it be possible to add some code comments explaining what the +1's and the -Nanosecond(1) are doing? Perhaps I'll learn the things needed to not regress again 😅.

let me take a stab at explaining, if this makes sense I can add comments for 2 and 3 (1 is just following the updated docs naively, so probably don't need a comment there):

  1. the + 1 change in time_from_index: I think the +1 in time_from_index was special-cased to not apply to the i:i case out of some notion that having an "instant-like" timespan in this case was desirable. however, i think that's just kind of inconsistent for no benefit as far as I can tell; not useful anywhere, and so probably just a naive design choice in the first place

  2. the + 1 in index_from_time: This +1 here isn't a change from main. But IIUC this +1 essentially converts from "offset-based" indexing (i.e. 0-based indexing) to "count-based" indexing (i.e. Julia's 1-based indexing)

  3. the - Nanosecond(1) change in index_from_time: AFAICT the underlying problem in index_from_time that wasn't being "directly accounted for" is that TimeSpans.stop(span) returns span's exclusive upper bound, but the calculation desires the span's inclusive upper bound (so as to not potentially "include" a sample point that shouldn't be included). On main, this is accounted for "indirectly"/"after-the-fact", but that solution isn't completely correct; so here, we instead try to just use the correct time point from the get-go, which is the final nanosecond included in the span (i.e. TimeSpans.stop(span) - Nanosecond(1)).