beacon-biosignals / alignedspans.jl Goto Github PK

This would be https://github.com/beacon-biosignals/TimeSpans.jl/blob/9f80d852a51b6df3d263b848c51c306c0d43fa40/src/TimeSpans.jl#L311-L365C72 but with a new shortest_aligned_span_containing instead of shortest_timespan_containing, and with checks that all spans have the same sample rate, and using by=x -> x.first_index instead of by=start where applicable.

If indeed all spans have the same sample rate, this should be the same as

merge_aligned_spans(f, aligned_spans) = AlignedSpan.(sample_rate, merge_spans(f, aligned_spans))

except more efficient and such that f gets aligned spans rather than timespans (or a mix of aligned spans and timespans).

Alternatively, we could try to update TimeSpans.jl to allow shortest_timespan_containing to be configurable in merge_spans, and could pass in our own shortest_aligned_span_containing instead. (And just pay the time for computing times with start rather than first_index, but that's probably fine).

function TimeSpans.translate(span::AlignedSpan, t::TimePeriod)
    n = AlignedSpans.n_samples(span.sample_rate, abs(t)) * sign(t)
    return AlignedSpan(span.sample_rate, span.first_index + n, span.last_index + n)
end

The fallback works but outputs a TimeSpan, so you may need to round again to get back. This also has the property of always preserving the number of samples contained in the span, which maybe it's possible to get wrong with pathological rounding otherwise.

For example, in the Motivation section, we have

julia> samples = Samples(permutedims(0:10), SamplesInfo("feature", ["a"], "microvolt", 0.5, 0.0, UInt16, sample_rate), false)
ERROR: UndefVarError: SamplesInfo not defined

Presumably this is because Onda now has SamplesInfoV1 and SamplesInfoV2.

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.

I bailed on this in #2 since I wanted to think about it more and wasn’t sure it was needed. I think there are times it’s pretty useful, and that there’s a simple thing we can do:

We define the start as time_from_index(sample_rate, span.first_index) and the stop as time_from_index(sample_rate, span.last_index + 1). In other words, if we had sample rate 1 and indices 5:10, we have AlignedSpan(1, 5, 10) which we would map to TimeSpan(Second(5), Second(11)).

In other words, we’re saying that 1 sample is associated to the time of the sample (inclusive) to the time of the next sample (exclusive).

Why?

• should allow roundtripping on both TimeSpans v0.2 and v0.3, with either RoundInwards or RoundEndsDown
• gives a useful duration; 5 samples @ 1Hz is 5s

Why do we ever need to go back to continuous time? All the cases I’ve found have essentially been to allow interop with code that expects TimeSpans-compatible spans. For example, for use with the Legolas schema for Onda Annotations. I think we should try to avoid mapping back and forth too much, but it’s nice if we can in a principled way.

To close this:

• define TimeSpans.start(::AlignedSpan) and TimeSpans.stop(::AlignedSpan) as above

• Add roundtripping tests (TimeSpan -> AlignedSpan -> TimeSpan only needs to be exact if the original times aligned on sample boundaries already, but AlignedSpan -> TimeSpan -> AlignedSpan should always be exact)

• Decide if we still want our own duration (Maybe? See #2)

• Decide if we want to show continuous time in our show method

• Test use in Onda Annotation?

• Decide if we need to test on TimeSpans v0.2 and v0.3. If so need to deal w Onda compat.

This issue is used to trigger TagBot; feel free to unsubscribe.

If you haven't already, you should update your TagBot.yml to include issue comment triggers.
Please see this post on Discourse for instructions and more details.

If you'd like for me to do this for you, comment TagBot fix on this issue.
I'll open a PR within a few hours, please be patient!

Consider adding keep_last to consecutive_overlapping_subspans, or at least filing it as a feature request?

Originally posted by @hannahilea in #19 (review)

I think it is a bit hard to know exactly what those semantics should be when there's overlap. I guess something like: if there's 8 fold overlap, the last 7 will all be shorter than usual, and the last one will be 1 sample long. But it's a bit weird. I'd rather leave that until someone needs it.

@ericphanson in #19 (comment)

If you need this feature, here's a good place to say so!