search

beacon-biosignals / AlignedSpans.jl

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

re-organize on top of Intervals.jl #2

Closed ericphanson closed 2 years ago

ericphanson commented 2 years ago

This is pointed at #1.

This drops support for TimeSpan(::AlignedSpan)-- this is done hackily in #1 by adding a single nanosecond to make the stop technically exclusive, but this isn't correctly supported by TimeSpans v0.2, and TimeSpans v0.3 isn't compatible with Onda yet (and it's not easy to update... I tried).

I think it's better to not support that, but rather support Interval(::AlignedSpan) (ref https://github.com/beacon-biosignals/TimeSpans.jl/issues/2). This lets us specify the openness/closedness of the endpoints, avoiding the need to fake an exclusive right-endpoint. In the spirit of that issue, TimeSpans.jl is no longer used in any crucial manner here; we just support constructing AlignedSpans from them, but don't use any of it's internal conversions methods (though we copy/modify some of them).

Onda support is added by providing a dispatch for Onda.column_arguments. This is much more direct than how it was going to be supported in #1, which requires converting to a TimeSpan first (i.e. the "generic" way by supporting TimeSpan construction is TimeSpan -> AlignedSpan -> TimeSpan -> indices, which is "continuous" -> "discrete" -> "continuous" -> "discrete" 😱 whereas now we go TimeSpan -> AlignedSpan -> indices, which is "continuous" -> "discrete" -> "discrete"). However, it does mean we need the Onda dependency.

I also added support for JSON and Arrow serialization, and cleaned up the API. We now provide a public interface for constructing an AlignedSpan from a continuous time representation; specific methods are provided for Intervals.Interval, and the fallback assumes TimeSpan's start and stop are defined.

@palday I've merged your review suggestions from #1 into here.

ericphanson commented 2 years ago

Preview docs are up: https://laughing-journey-cc7584d6.pages.github.io/previews/PR2/

ericphanson commented 2 years ago

I almost want to define

start_time(span::AlignedSpan) = time_from_index(span.sample_rate, span.first_index)
stop_time(span::AlignedSpan) = time_from_index(span.sample_rate, span.last_index+1) - Nanosecond(1)
duration(span::AlignedSpan) = stop_time(span) - start_time(span) + Nanosecond(1)

which looks kind of unhinged, but that way n_samples(aligned_span.sample_rate, duration(aligned_span)) == n_samples(aligned_span) in a lot of cases...

ericphanson commented 2 years ago

I think my remaining issues / uncertainty with AlignedSpans are all about going from AlignedSpans back to a continuous representation. I think this PR has the "cts -> discrete" path well-defined and figured out.

That makes me think for this PR, I should just drop all the "discrete -> cts" stuff (stop_time, duration(::AlignedSpan)), define a different show type that doesn't imply an embedding back into continuous time, and then leave that stuff for future work.

The main purpose anyway is having clear/explicit "cts -> discrete" transformations, which this does.