beacon-biosignals / AlignedSpans.jl

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

Support TimeSpans interface to go back to continuous time #9

Closed ericphanson closed 2 years ago

ericphanson commented 2 years ago

closes #7

Decided to use TimeSpans.duration instead of our own duration function here (in contrast to in #2), since now AlignedSpans are TimeSpans.jl-compatible timespans, so it's weird for them to have their own duration.

I think this is slightly breaking because TimeSpans.istimespan changed values, and now we use TimeSpans.duration instead of AlignedSpans.duration.

editorial note: I'm pretty excited about this bc I think it's the "right" choice (finally), but most likely only time/usage will be able to really tell if it's "good".


Some more background for reviewers:

TimeSpans.jl is a library for defining an interface for representing continous time spans. It also has some functionality for getting indicies out (index_from_time, time_from_index). It will probably help to see the TimeSpans docs, and TimeSpans#26 ,and maybe TimeSpans#2. It could also help to check out #2.

codecov[bot] commented 2 years ago

Codecov Report

Merging #9 (cb2eab6) into main (6e701ac) will increase coverage by 1.61%. The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main       #9      +/-   ##
==========================================
+ Coverage   92.75%   94.36%   +1.61%     
==========================================
  Files           4        4              
  Lines          69       71       +2     
==========================================
+ Hits           64       67       +3     
+ Misses          5        4       -1     
Impacted Files Coverage Δ
src/AlignedSpans.jl 94.11% <100.00%> (+0.36%) :arrow_up:
src/interop.jl 89.65% <100.00%> (+3.94%) :arrow_up:

:mega: Codecov can now indicate which changes are the most critical in Pull Requests. Learn more

ericphanson commented 2 years ago

@palday rm'd the request for your review to comply with our <= 2 reviewers policy, but your feedback is always appreciated of course :)

ericphanson commented 2 years ago

Thanks for the review @franzfurbass! Regarding RoundEndsDown, I didn't go with RoundDown, since that's already exported by Base for things like

julia> round(Int, 1.8, RoundDown)
1

We could use the same object to represent rounding both endpoints down, but I thought that was kind of a bad pun to use RoundDown for both rounding a single endpoint down and for rounding both. For example, in EndpointRoundingMode(RoundDown, RoundUp), the meaning of RoundDown is the same as it is in round(Int, 1.8, RoundDown): it's rounding a single number down. But I thought it would be weird for EndpointRoundingMode(RoundDown, RoundDown) to be the same as just RoundDown.

To me, I hoped plural Ends would make it clear that it's both "ends" of the interval, not just the stop time. But if it's confusing then we should have a better name.

What about RoundEndpointsDown? Is that more clear?

franzfurbass commented 2 years ago

What about RoundEndpointsDown? Is that more clear?

The thing that dropped my out was the "endpoint" in the name. The rounding mode of TimeSpans rounds down the startpoint and the endpoint of the timespan. This naming convention is also used for EndpointRoundingMode, on a second thought that has the same issue.

maybe SampleRoundingMode for EndpointRoundingMode RoundTimepointsDown fro RoundEndsDown

ericphanson commented 2 years ago

What about SpanRoundingMode for EndpointRoundingMode, and RoundSpanDown for RoundEndsDown?

franzfurbass commented 2 years ago

Sounds good!

ericphanson commented 2 years ago

@franzfurbass if you're happy with the PR now, can you leave an "approval" review? That does 2 things:

  1. it clears the "changes requested", Screen Shot 2022-03-30 at 16 15 40
  2. and says it's OK for me to merge. Technically, I could merge anyway since I'm an admin on the repo, but we generally don't use that power and instead wait for an approving review: Screen Shot 2022-03-30 at 16 16 28