Add rounding support for `Periods` #24182

spurll · 2017-10-17T16:11:31Z

Allows (most) subtypes of Period to be rounded with floor, ceil, and round, also allowing for easy conversion between Period subtypes where we would otherwise encounter an InexactError.

julia> x = Dates.Millisecond(29394943)
29394943 milliseconds

julia> convert(Dates.Hour, x)
ERROR: InexactError: divexact(Int64, 8.165261944444444)
Stacktrace:
 [1] divexact(::Int64, ::Int64) at ./dates/periods.jl:400
 [2] convert(::Type{Base.Dates.Hour}, ::Base.Dates.Millisecond) at ./dates/periods.jl:422

julia> round(x, Dates.Hour)
8 hours

As with the date rounding functions, this approach also supports rounding to values (e.g., the nearest 15 minutes) as well as types:

julia> x = Dates.Hour(910)
910 hours

julia> ceil(x, Dates.Hour(12))
912 hours

I can see at least two potential concerns being raised about this approach:

As discussed in trunc/floor/ceil/round interface inconsistency for methods involving dates? #18574, there may be some confusion/contention surrounding the typical rounding signature (round(value, precision)) and the convert signature (convert(type, value)).
For certain very large values, the rounding may still result in an InexactError. This is because all values are converted to Nanosecond for rounding, so we can't easily accommodate values that exceed typemax(Int64) nanoseconds (which is a little over 292 years). This could be mitigated by using a lower-precision Period type when nanosecond precision is not required.

Edit: Point 2 above has been mitigated via promote.

iamed2 · 2017-10-17T16:30:02Z

Could/should these be round(Dates.Hour, x) and round(x, Dates.Hour(12)) to match the round(type, value) and round(value, precision) methods? You could also conceivably have round(Dates.Minute, x, Dates.Hour(12)) though I'm not sure how useful that would be.

spurll · 2017-10-17T16:41:41Z

Unless something has changed, these signatures actually do match the signatures of the date rounding functions, but the confusion is completely understandable (this is actually addressed in the first point of contention above).

spurll · 2017-10-17T16:49:18Z

To clarify, both the "round a date(time) to a period value" and "round a date(time) to a period type" have the same signature, which is round(val, precision), so this uses the same interface.

But I could definitely see an argument for "round to a period value" and "round to a period type" having different interfaces (with the latter using the same order as convert, as you suggest). I'm not sure whether that would lead to more or less confusion.

spurll · 2017-10-17T16:52:55Z

The initial decision to use the rounding signature we currently have for date(time)s was informed by trunc, which already existed at the time. But to add to the confusion, trunc has a kind of inconsistent signature.

Generally, we have trunc(T::Type, x). But for TimeTypes we have trunc(dt::TimeType, ::Type{Period}).

So... this is why this is an RFC. 😁

iamed2 · 2017-10-17T17:29:23Z

But I could definitely see an argument for "round to a period value" and "round to a period type" having different interfaces (with the latter using the same order as convert, as you suggest). I'm not sure whether that would lead to more or less confusion.

This is the argument I was trying to make 😄

spurll · 2017-10-17T19:43:41Z

I could definitely go either way on that one. But if we move to:

round(precision::Type{Period}, x)
round(x, precision::Period)

then the interfaces for both trunc and the existing date rounding functions should be updated accordingly.

iamed2 · 2017-10-17T20:00:52Z

We should see what @omus thinks

omus · 2017-10-18T16:05:13Z

I think round for Period reminds me more of the mod behaviour rather than just indicating the return type:

julia> mod(260,256)
4

julia> mod(260,UInt8)
0x04

spurll · 2017-10-26T19:20:17Z

That's interesting. So you're in favour of keeping the existing signatures, @omus?

omus · 2017-10-26T19:46:39Z

Yes, I think the existing signatures are reasonable

omus · 2017-10-26T19:50:49Z

base/dates/rounding.jl


 # Make rounding functions callable using Period types in addition to values.
-Base.floor(dt::TimeType, p::Type{<:Period}) = Base.floor(dt, p(1))
-Base.ceil(dt::TimeType, p::Type{<:Period}) = Base.ceil(dt, p(1))
+Base.floor(x::TimeTypePeriod, p::Type{<:Period}) = Base.floor(x, p(1))


Should probably be changed to:

Base.floor(x::TimeTypePeriod, ::Type{P}) where P <: Period = Base.floor(x, oneunit(P))

Interesting. I was completely unaware of oneunit.

It's the unit aware sibling to one

omus · 2017-10-26T19:52:08Z

test/dates/rounding.jl

+end
+@testset "Rounding for periods that should not need rounding" begin
+    for x in [Dates.Week(3), Dates.Day(14), Dates.Microsecond(604800000000)]
+        local dt


I should not be allowed to use copy/paste.

omus · 2017-10-26T19:52:16Z

test/dates/rounding.jl

+    for x in [Dates.Week(3), Dates.Day(14), Dates.Microsecond(604800000000)]
+        local dt
+        for p in [Dates.Week, Dates.Day, Dates.Hour, Dates.Second, Dates.Millisecond, Dates.Microsecond, Dates.Nanosecond]
+            local p


@kshyatt can you elaborate on why this is needed?

This is a case where I noticed that local dt had been added to the tests in several places, so I replicated that syntax for these tests (in this case local dt should actually be local x above). But yeah, I don't actually know what those lines are meant to accomplish.

omus · 2017-10-26T19:55:16Z

base/dates/rounding.jl

@@ -7,6 +7,9 @@ const DATETIMEEPOCH = value(DateTime(0))
 # According to ISO 8601, the first day of the first week of year 0000 is 0000-01-03
 const WEEKEPOCH = value(Date(0, 1, 3))

+const ConvertiblePeriod = Union{TimePeriod, Week, Day}
+const TimeTypePeriod = Union{TimeType, ConvertiblePeriod}


Maybe rename to TimeTypeOrPeriod

omus

Looks good. I'd like to know what the local loop variables are for yet.

omus · 2017-10-27T18:13:01Z

Maybe spoke too soon @spurll there looks like there is a 32-bit issue.

spurll · 2017-10-27T19:43:36Z

Ah yes. It looks like Nanosecond is using Int under the hood rather than Int64, so I'll have to address that.

Period rounding functions no longer unnecessarily convert all values to nanoseconds prior to rounding.

spurll · 2017-10-29T17:00:25Z

AppVeyor failure seems unrelated. I believe everything else is fixed.

omus · 2017-10-29T20:33:02Z

base/dates/rounding.jl

+"""
+function Base.floor(x::ConvertiblePeriod, precision::T) where T <: ConvertiblePeriod
+    value(precision) < 1 && throw(DomainError(precision))
+    x, precision = promote(x, precision)


Should use a different variable name for the assignment to ensure type stability

omus · 2017-10-29T20:34:12Z

base/dates/rounding.jl

+function Base.round(x::ConvertiblePeriod, precision::ConvertiblePeriod, r::RoundingMode{:NearestTiesUp})
+    f, c = floorceil(x, precision)
+    common_x, common_f, common_c = promote(x, f, c)
+    return (common_x - common_f) < (common_c - common_x) ? f : c


I think _x, _f, _c = ... is fine if you want to use a more concise name

omus · 2017-10-29T20:35:03Z

base/dates/rounding.jl

-function Base.round(dt::TimeType, p::Type{<:Period}, r::RoundingMode=RoundNearestTiesUp)
-    return Base.round(dt, p(1), r)
+function Base.round(x::TimeTypeOrPeriod, ::Type{P}, r::RoundingMode=RoundNearestTiesUp) where P <: Period
+     return Base.round(x, oneunit(P), r)


Extra space

spurll · 2017-10-30T16:36:35Z

I'm not sure what's going on with CI here.

omus · 2017-11-01T22:46:00Z

Looks good. I'm going to retry the CI.

kshyatt added the dates Dates, times, and the Dates stdlib module label Oct 17, 2017

spurll changed the title ~~RFC: Add rounding support for Periods~~ Add rounding support for Periods Oct 26, 2017

Add rounding for Periods

586d793

omus requested changes Oct 26, 2017

View reviewed changes

spurll force-pushed the gn/period-rounding branch from b0c87ad to 586d793 Compare October 26, 2017 19:55

Improve period rounding function type signatures

f1f0194

omus approved these changes Oct 27, 2017

View reviewed changes

spurll changed the title ~~Add rounding support for Periods~~ WiP: Add rounding support for Periods Oct 27, 2017

Use promote instead of Nanosecond to round periods

6cd95a0

Period rounding functions no longer unnecessarily convert all values to nanoseconds prior to rounding.

spurll changed the title ~~WiP: Add rounding support for Periods~~ Add rounding support for Periods Oct 27, 2017

omus requested changes Oct 29, 2017

View reviewed changes

Minor refactoring for period rounding

ff158ec

omus approved these changes Nov 1, 2017

View reviewed changes

omus closed this Nov 1, 2017

omus reopened this Nov 1, 2017

omus merged commit f16a119 into JuliaLang:master Nov 2, 2017

omus deleted the gn/period-rounding branch November 2, 2017 12:39

George9000 mentioned this pull request Jun 19, 2021

InexactError when dividing or multiplying Dates.Millisecond #15322

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add rounding support for `Periods` #24182

Add rounding support for `Periods` #24182

spurll commented Oct 17, 2017 •

edited

Loading

iamed2 commented Oct 17, 2017

spurll commented Oct 17, 2017 •

edited

Loading

spurll commented Oct 17, 2017 •

edited

Loading

spurll commented Oct 17, 2017 •

edited

Loading

iamed2 commented Oct 17, 2017

spurll commented Oct 17, 2017 •

edited

Loading

iamed2 commented Oct 17, 2017

omus commented Oct 18, 2017

spurll commented Oct 26, 2017

omus commented Oct 26, 2017

omus Oct 26, 2017

spurll Oct 26, 2017

omus Oct 26, 2017

omus Oct 26, 2017

spurll Oct 26, 2017

omus Oct 26, 2017

omus Oct 26, 2017

spurll Oct 27, 2017

omus Oct 26, 2017

omus left a comment

omus commented Oct 27, 2017

spurll commented Oct 27, 2017

spurll commented Oct 29, 2017

omus Oct 29, 2017

omus Oct 29, 2017

omus Oct 29, 2017

spurll commented Oct 30, 2017

omus commented Nov 1, 2017

Add rounding support for Periods #24182

Add rounding support for Periods #24182

Conversation

spurll commented Oct 17, 2017 • edited Loading

iamed2 commented Oct 17, 2017

spurll commented Oct 17, 2017 • edited Loading

spurll commented Oct 17, 2017 • edited Loading

spurll commented Oct 17, 2017 • edited Loading

iamed2 commented Oct 17, 2017

spurll commented Oct 17, 2017 • edited Loading

iamed2 commented Oct 17, 2017

omus commented Oct 18, 2017

spurll commented Oct 26, 2017

omus commented Oct 26, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

omus left a comment

Choose a reason for hiding this comment

omus commented Oct 27, 2017

spurll commented Oct 27, 2017

spurll commented Oct 29, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

spurll commented Oct 30, 2017

omus commented Nov 1, 2017

Add rounding support for `Periods` #24182

Add rounding support for `Periods` #24182

spurll commented Oct 17, 2017 •

edited

Loading

spurll commented Oct 17, 2017 •

edited

Loading

spurll commented Oct 17, 2017 •

edited

Loading

spurll commented Oct 17, 2017 •

edited

Loading

spurll commented Oct 17, 2017 •

edited

Loading