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

`start` and `stop` shouldn't be defined for TimePeriod #40

Open ericphanson opened 2 years ago

ericphanson commented 2 years ago

https://github.com/beacon-biosignals/TimeSpans.jl/blob/eb327966a96776b1b5f9980ad18e3719930d8279/src/TimeSpans.jl#L105-L119

I get that it's so that TimePeriod's act like a 1-ns range around that time, but I think it's overly error prone. E.g.

duration(time_from_index(fs, size(data, 2)))

gives 1 Nanosecond always, where the intended code is

 duration(time_from_index(fs, 1:size(data, 2)))

IMO in this case a clear error from the first example would be preferable.

ararslan commented 2 years ago

There is a potential alternate behavior of the TimeSpans interface for Periods that I think could arguably make more sense: start(period) is 0 and duration(period) == period. I have on at least one occasion forgotten about the one-nanosecond behavior and thought it worked this way. I think it would actually make your two time_from_index examples behave identically.

ericphanson commented 2 years ago

I think that kinda makes sense, but the ambiguity seems like a good argument that these should be MethodError's instead, so you have to specify