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

Unexpected results during `invert_spans` if a timespan is straddling in and out of the `parent_span` #52

Closed kimlaberinto closed 1 year ago

kimlaberinto commented 1 year ago

Semi-unexpected. I would've thought that inverting on collections that contained "straddling timespans" (i.e. spans where one end is in the parent_spanand where one end is not in parent_span) would still result in gaps that do not overlap with any TimeSpan in the input collection. Better explained with example: e.g.

julia> invert_spans([TimeSpan(-5, 3), TimeSpan(6, 8)], TimeSpan(0, 10))
2-element Vector{TimeSpan}:
 TimeSpan(00:00:00.000000000, 00:00:00.000000006)
 TimeSpan(00:00:00.000000008, 00:00:00.000000010)

In the above example, one might find the above unexpected as they would've thought that the function would return [TimeSpan(3, 6), TimeSpan(8, 10)] instead. Another reason why the above is unexpected is because that invert_spans can handle timespans are that completely outside the parent_span, so one might think that it can also handle straddling timespans.

julia> invert_spans([TimeSpan(2, 3), TimeSpan(100, 200)], TimeSpan(0, 10))
2-element Vector{TimeSpan}:
 TimeSpan(00:00:00.000000000, 00:00:00.000000002)
 TimeSpan(00:00:00.000000003, 00:00:00.000000010)

julia> invert_spans([TimeSpan(2, 3), TimeSpan(-200, -100)], TimeSpan(0, 10))
2-element Vector{TimeSpan}:
 TimeSpan(00:00:00.000000000, 00:00:00.000000002)
 TimeSpan(00:00:00.000000003, 00:00:00.000000010)

Alternatively though: The other perspective is maybe invert_spans isn't even meant to work on straddling timespans or timespans outside the input parent_span.


I originally ran into this by hitting an error when running on a 1-element collection whose sole timespan is a straddling timespan (or even completely outside).

julia> invert_spans([TimeSpan(100, 200)], TimeSpan(0, 10))
ERROR: BoundsError: attempt to access 0-element Vector{TimeSpan} at index [1]
Stacktrace:
 [1] getindex
   @ ./array.jl:924 [inlined]
 [2] first
   @ ./abstractarray.jl:404 [inlined]
 [3] invert_spans(spans::Vector{TimeSpan}, parent_span::TimeSpan)
   @ TimeSpans ~/.julia/packages/TimeSpans/eDfrp/src/TimeSpans.jl:371
 [4] top-level scope
   @ REPL[5]:1

julia> invert_spans([TimeSpan(-5, 5)], TimeSpan(0, 10))
ERROR: BoundsError: attempt to access 0-element Vector{TimeSpan} at index [1]
... (same as above)

IMO to address this issue, we'll have to determine what behaviour we want TimeSpans.jl to do in these cases

ararslan commented 1 year ago

In the above example, one might find the above unexpected as they would've thought that the function would return [TimeSpan(3, 6), TimeSpan(8, 10)] instead.

I know I certainly would have thought that. This seems like a bug to me.

ararslan commented 1 year ago

Ah, it's because of this line: https://github.com/beacon-biosignals/TimeSpans.jl/blob/main/src/TimeSpans.jl#L378

Rather than proper containment within the parent span, we should check for overlap with the parent span.

hannahilea commented 1 year ago

BIG yikes---and good find/mwe!!