Support TimeSpans interface to go back to continuous time #9

ericphanson · 2022-03-29T11:59:46Z

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 · 2022-03-29T12:02:50Z

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%)`	⬆️
src/interop.jl	`89.65% <100.00%> (+3.94%)`	⬆️

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

docs/src/index.md

ericphanson · 2022-03-29T17:24:46Z

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

franzfurbass

Nice job! This is definetly an important issue. I tried my best to understand all aspects.
I made some suggestions to the documenation to be easier to understand for first-time-users.
One rounding mode seemed to have a missleading name to me, but I am not shure if it is possible to change that due already existing symbol names. One of the tests seems to have a glitch.

Project.toml

README.md

franzfurbass · 2022-03-30T08:07:36Z

docs/src/index.md

+
+* `EndpointRoundingMode`: consists of a `RoundingMode` for the `start` and `stop` of the span.
+    * The alias `RoundInward = EndpointRoundingMode(RoundUp, RoundDown)`, for example, constructs the largest span (whose endpoints are valid indices) that is entirely contained within `span`.
+    * The alias `RoundEndsDown = EndpointRoundingMode(RoundDown, RoundDown)` matches the rounding semantics of `TimeSpans.index_from_time(sample_rate, span)`.


Suggested change

* The alias `RoundEndsDown = EndpointRoundingMode(RoundDown, RoundDown)` matches the rounding semantics of `TimeSpans.index_from_time(sample_rate, span)`.

* The alias `RoundDown = EndpointRoundingMode(RoundDown, RoundDown)` matches the rounding semantics of `TimeSpans.index_from_time(sample_rate, span)`.

When reading RoundEndsDown I expected that only the end time is rounded down. Maybe this alias is more intuitive.

Not shure if there is now a symbol clash as RoundDown was existing before?

docs/src/index.md

franzfurbass · 2022-03-30T08:24:21Z

src/AlignedSpans.jl

@@ -40,6 +43,11 @@ end
 const RoundInward = EndpointRoundingMode(RoundUp, RoundDown)
 const RoundEndsDown = EndpointRoundingMode(RoundDown, RoundDown)


Suggested change

const RoundEndsDown = EndpointRoundingMode(RoundDown, RoundDown)

const RoundDown = EndpointRoundingMode(RoundDown, RoundDown)

I am leaving the decision to rename to you. For me it seems more clear to use rounddown.
Other files will need that change too.

test/interop.jl

franzfurbass · 2022-03-30T08:35:12Z

test/interop.jl

+    for sample_rate in [1.0, 0.5, 100.0, 128.33]
+        # AlignedSpan -> TimeSpan -> AlignedSpan
+        for (i, j) in [1 => 10, 5 => 20, 3 => 6, 78 => 79]
+            for mode in (RoundEndsDown, RoundInward, ConstantSamplesRoundingMode)


Not shure if this is the correct test: should't mode used in constructing the reference object? Now RoundEndsDown is used always

Oops, that's right! Hopefully the test still passes 😅

It does not, so I had to weaken it a bit

test/interop.jl

test/time_index_conversions.jl

Co-authored-by: Franz Fürbass <[email protected]>

ericphanson · 2022-03-30T13:03:48Z

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 · 2022-03-30T13:19:52Z

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 · 2022-03-30T13:53:00Z

What about SpanRoundingMode for EndpointRoundingMode, and RoundSpanDown for RoundEndsDown?

franzfurbass · 2022-03-30T13:54:34Z

Sounds good!

ericphanson · 2022-03-30T14:18:21Z

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

it clears the "changes requested",

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:

franzfurbass

Sorry, forgot to submit the review. Thanks!

ericphanson added 2 commits March 29, 2022 13:30

add start/stop method

2a63bf4

use TimeSpans.duration, update docs

0d7ed70

ericphanson requested a review from palday March 29, 2022 11:59

ericphanson commented Mar 29, 2022

View reviewed changes

docs/src/index.md Show resolved Hide resolved

Update docs/src/index.md

5222432

ericphanson commented Mar 29, 2022

View reviewed changes

docs/src/index.md Outdated Show resolved Hide resolved

ericphanson added 2 commits March 29, 2022 17:40

Update docs/src/index.md

ffca616

fail docs if doctests fail

9aa3ad4

ericphanson requested review from franzfurbass and kendal-s and removed request for palday March 29, 2022 17:24

add some tests

5d9341a

franzfurbass requested changes Mar 30, 2022

View reviewed changes

ericphanson and others added 3 commits March 30, 2022 14:13

Apply suggestions from code review

eb745d1

Co-authored-by: Franz Fürbass <[email protected]>

add ascii diagrams

466dfeb

fix test, improve docs

3092d41

ericphanson requested a review from franzfurbass March 30, 2022 13:04

ericphanson added 2 commits March 30, 2022 15:58

rename EndpointRoundingMode -> SpanRoundingMode

678e4b1

rename RoundEndsDown -> RoundSpanDown

cb2eab6

franzfurbass reviewed Mar 30, 2022

View reviewed changes

franzfurbass approved these changes Mar 30, 2022

View reviewed changes

ericphanson merged commit d8a9dcc into main Mar 30, 2022

ericphanson deleted the eph/discrete_to_cts branch March 30, 2022 21:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support TimeSpans interface to go back to continuous time #9

Support TimeSpans interface to go back to continuous time #9

ericphanson commented Mar 29, 2022 •

edited

Loading

codecov bot commented Mar 29, 2022 •

edited

Loading

ericphanson commented Mar 29, 2022

franzfurbass left a comment

franzfurbass Mar 30, 2022

franzfurbass Mar 30, 2022

franzfurbass Mar 30, 2022

franzfurbass Mar 30, 2022

franzfurbass Mar 30, 2022

franzfurbass Mar 30, 2022

ericphanson Mar 30, 2022

ericphanson Mar 30, 2022 •

edited

Loading

ericphanson commented Mar 30, 2022

franzfurbass commented Mar 30, 2022

ericphanson commented Mar 30, 2022

franzfurbass commented Mar 30, 2022

ericphanson commented Mar 30, 2022

franzfurbass left a comment

	* The alias `RoundEndsDown = EndpointRoundingMode(RoundDown, RoundDown)` matches the rounding semantics of `TimeSpans.index_from_time(sample_rate, span)`.
	* The alias `RoundDown = EndpointRoundingMode(RoundDown, RoundDown)` matches the rounding semantics of `TimeSpans.index_from_time(sample_rate, span)`.

		@@ -40,6 +43,11 @@ end
		const RoundInward = EndpointRoundingMode(RoundUp, RoundDown)
		const RoundEndsDown = EndpointRoundingMode(RoundDown, RoundDown)

	const RoundEndsDown = EndpointRoundingMode(RoundDown, RoundDown)
	const RoundDown = EndpointRoundingMode(RoundDown, RoundDown)

Support TimeSpans interface to go back to continuous time #9

Support TimeSpans interface to go back to continuous time #9

Conversation

ericphanson commented Mar 29, 2022 • edited Loading

codecov bot commented Mar 29, 2022 • edited Loading

Codecov Report

ericphanson commented Mar 29, 2022

franzfurbass left a comment

Choose a reason for hiding this comment

franzfurbass Mar 30, 2022

Choose a reason for hiding this comment

franzfurbass Mar 30, 2022

Choose a reason for hiding this comment

franzfurbass Mar 30, 2022

Choose a reason for hiding this comment

franzfurbass Mar 30, 2022

Choose a reason for hiding this comment

franzfurbass Mar 30, 2022

Choose a reason for hiding this comment

franzfurbass Mar 30, 2022

Choose a reason for hiding this comment

ericphanson Mar 30, 2022

Choose a reason for hiding this comment

ericphanson Mar 30, 2022 • edited Loading

Choose a reason for hiding this comment

ericphanson commented Mar 30, 2022

franzfurbass commented Mar 30, 2022

ericphanson commented Mar 30, 2022

franzfurbass commented Mar 30, 2022

ericphanson commented Mar 30, 2022

franzfurbass left a comment

Choose a reason for hiding this comment

ericphanson commented Mar 29, 2022 •

edited

Loading

codecov bot commented Mar 29, 2022 •

edited

Loading

ericphanson Mar 30, 2022 •

edited

Loading