Remove problematic Dates conversions #19920

timholy · 2017-01-07T20:16:13Z

Fixes #19896. This doesn't quite pass tests; we should discuss what the names for conversions between reals and Date/DateTime should be (see the deprecations). CC @quinnj.

timholy · 2017-01-07T20:17:59Z

base/deprecated.jl

+@deprecate convert{R<:Real}(::Type{R},x::Dates.DateTime) R(Dates.datetime2rata(x))
+@deprecate convert{R<:Real}(::Type{R},x::Dates.Date)     R(Dates.datetime2rata(x))
+@deprecate convert(::Type{Dates.DateTime}, x::Real) Dates.rata2datetime(x)
+@deprecate convert(::Type{Dates.Date}, x::Real)     Dates.Date(Dates.rata2datetime(x))


It's the previous 4 lines that need particular scrutiny and deciding.

I think these are probably fine. The first two could be Dates.value(x), but it's probably more consistent to just use the rata2datetime and datetime2rata conversions.

I think the one intended to replace convert(Date, x) gives the wrong answer, though. Isn't it something like milliseconds vs days or something? (I apologize for not knowing as much as I should about the Dates infrastructure, it's clearly carefully thought out and I feel a bit like a bull in a well-designed china shop...)

Bump @quinnj.

GitHub seems to have messed up the threading order here (EDIT: only on "changes" view. Weird.): the comments were made in the order me, @quinnj, and then my comment about convert(Date, x) giving the wrong answer in response to @quinnj. In other words, the problem I highlighted is still a concern (it's why the tests fail for this PR).

What should the API actually look like? What functions should we export?

Awesome, thanks for the help folks! I used these as a guideline to rework the modified tests at the bottom of test/dates/conversions.jl, here are the results:

julia> @test Dates.datetime2rata(a) == 63082368000000 Test Failed Expression: Dates.datetime2rata(a) == 63082368000000 Evaluated: 730120 == 63082368000000 ERROR: There was an error during testing julia> @test Dates.UTD(63082368000000) == a Test Failed Expression: Dates.UTD(63082368000000) == a Evaluated: Base.Dates.UTInstant{Base.Dates.Day}(63082368000000 days) == 2000-01-01T00:00:00 ERROR: There was an error during testing julia> @test Dates.UTD(63082368000000.0) == a Test Failed Expression: Dates.UTD(6.3082368e13) == a Evaluated: Base.Dates.UTInstant{Base.Dates.Day}(63082368000000 days) == 2000-01-01T00:00:00 ERROR: There was an error during testing julia> @test Dates.UTM(730120) == b Test Failed Expression: Dates.UTM(730120) == b Evaluated: Base.Dates.UTInstant{Base.Dates.Millisecond}(730120 milliseconds) == 2000-01-01 ERROR: There was an error during testing julia> @test Dates.UTM(730120.0) == b Test Failed Expression: Dates.UTM(730120.0) == b Evaluated: Base.Dates.UTInstant{Base.Dates.Millisecond}(730120 milliseconds) == 2000-01-01 ERROR: There was an error during testing julia> @test Dates.UTM(Int32(730120)) == b Test Failed Expression: Dates.UTM(Int32(730120)) == b Evaluated: Base.Dates.UTInstant{Base.Dates.Millisecond}(730120 milliseconds) == 2000-01-01 ERROR: There was an error during testing

I may have screwed some of those up, but it appears we aren't there yet.

I tracked down the issue here. We need to change the deprecated conversions to:

@deprecate convert{R<:Real}(::Type{R},x::Dates.DateTime) R(Dates.value(x)) @deprecate convert{R<:Real}(::Type{R},x::Dates.Date) R(Dates.value(x)) @deprecate convert(::Type{Dates.DateTime}, x::Real) Dates.DateTime(Dates.UTM(x)) @deprecate convert(::Type{Dates.Date}, x::Real) Dates.Date(Dates.UTD(x))

The conversion from DateTime to Real was incorrect as it was working in Rata Die days instead of milliseconds. The tests at the end of test/dates/conversions.jl should look like:

# Conversions to/from numbers a = Dates.DateTime(2000) b = Dates.Date(2000) @test Dates.value(b) == 730120 @test Dates.value(a) == 63082368000000 @test Dates.DateTime(Dates.UTM(63082368000000)) == a @test Dates.DateTime(Dates.UTM(63082368000000.0)) == a @test Dates.Date(Dates.UTD(730120)) == b @test Dates.Date(Dates.UTD(730120.0)) == b @test Dates.Date(Dates.UTD(Int32(730120))) == b

I would suggest that we add the following methods:

Base.convert(::Type{DateTime}, x::Millisecond) = DateTime(Dates.UTInstant(x)) # Converts Rata Die milliseconds to a DateTime Base.convert(::Type{Date}, x::Day) = Date(Dates.UTInstant(x)) # Converts Rata Die days to a Date

That way we can have a reasonable way of converting Reals into DateTimes and Dates:

julia> convert(Date, Day(730120)) 2000-01-01 julia> convert(DateTime, Millisecond(63082368000000)) 2000-01-01T00:00:00

Should the reverse conversions also be defined?

I think both conversions to and from Day/Millisecond are great.

StefanKarpinski · 2017-01-08T16:16:07Z

At a higher level, almost all of the actual complexity of date and time stuff should not be in Base Julia – just the basic type definitions, printing and a little basic arithmetic. Everything else should be in a DateTime package which adds all this conversion and other functionality. Base should contain the minimal amount of date time code to allow Base functions to return (and take, but mostly return, since that's the thing we can't externally overload) date and time values.

TotalVerb · 2017-01-09T00:59:03Z

This probably also fixes #10451.

tkelman · 2017-01-19T17:22:24Z

Bump, can we get this finished up before (imminent) feature freeze?

timholy · 2017-01-19T20:02:27Z

It's waiting on feedback from someone who actually knows about how Date/Time works. The key question is what the API should look like for conversions between "unitful" and "unitless" measures of time (which I presume are important for interoperation with other systems/software).

omus

Overall looks reasonable.

omus · 2017-01-23T21:03:56Z

base/dates/periods.jl

 """
 function default end

-default{T<:DatePeriod}(p::Union{T,Type{T}}) = one(p)
+default{T<:DatePeriod}(p::Union{T,Type{T}}) = T(1)
 default{T<:TimePeriod}(p::Union{T,Type{T}}) = zero(p)


Shouldn't this be T(0)? At the very least it doesn't match the comment above.

I think both give the same answer.

They do give the same answer. Just appears inconsistent

omus · 2017-01-23T21:18:01Z

base/dates/ranges.jl

-guess(a::DateTime,b::DateTime,c) = floor(Int64,(Int128(b) - Int128(a))/toms(c))
-guess(a::Date,b::Date,c) = Int64(div(Int64(b - a),days(c)))
+guess(a::DateTime,b::DateTime,c) = floor(Int64,(Int128(value(b)) - Int128(value(a)))/toms(c))
+guess(a::Date,b::Date,c) = Int64(div(Int64(value(b - a)),days(c)))


Should be safe to remove the inner Int64 call.

omus · 2017-01-23T21:24:00Z

test/dates/ranges.jl

@@ -482,10 +482,10 @@ function testmonthranges2(n)
 end
 testmonthranges2(100000)

-@test length(Dates.Year(1):Dates.Year(10)) == 10
+@test length(Dates.Year(1):Dates.Year(1):Dates.Year(10)) == 10


Why do we need to specify the step here? Can't we just define the following to get the old behaviour?

Base.colon{T<:Period}(start::T, stop::T) = StepRange(start, T(1), stop)

I think it's dangerous to define ranges with unitful quantities if you don't explicitly specify the step, because with unitful quantities we tend to think that there's an underlying physical reality independent of the units you choose to express the quantities in. Why should 1m:10m give a range of length 10 whereas 1000mm:10000mm gives you a range of length 9001? Both of those ranges are defined with the same endpoints.

In my mind the only reasonable approach is to force the user to specify the step or the desired length of the range.

I'm fine with requiring the user to specify the step, but this change should be documented.

In principle I agree that you should define step for unitful ranges. However since ranges involving periods currently require that the endpoints and the step are of the same unit I think this change is mostly just inconvenient. Overall though I'm happy with the change if it leads to less confusion.

I can see the sense in supporting it when you're typing the endpoints explicitly: I think it's pretty obvious what the user wants with Minute(1):Minute(60). But I worry about code like this:

function foo(a::Period, b::Period) ap, bp = promote(a, b) ap:bp end

where now knowing what you'll get relies on a fair amount of mental gymnastics.

Either way, I agree completely with the perspective that we need a transitional strategy. I suspect a deprecation would suffice?

timholy · 2017-01-23T22:30:46Z

Thanks for the review, @omus! If you can help with #19920 (comment), that's really the key issue. Do we need a new name for one of the conversions? What functions should we export?

StefanKarpinski · 2017-01-24T01:25:01Z

What's needed to move this forward?

omus · 2017-01-24T01:36:02Z

@StefanKarpinski I believe this is the only blocking change. Should also add some documentation regarding requiring the step for Period ranges. The rest are minor.

omus · 2017-01-24T01:37:38Z

My comment above still stands. I was attempting to resolve my previous suggestions.

timholy · 2017-01-24T17:57:16Z

Updated. Thanks for the suggestions!

tkelman · 2017-01-24T19:07:05Z

base/dates/ranges.jl

+Base.range(start::DateTime, len::Integer)  = range(start, Day(1), len)
+Base.range(start::Date, len::Integer)      = range(start, Day(1), len)
+
+(::Type{StepRange{T,R}}){T<:Dates.DatePeriod,R<:Real}(start, step, stop) = throw(ArgumentError("must specify step as a Period when constructing Dates ranges"))


…n ranges Fixes #19896

tkelman · 2017-01-26T21:19:51Z

test/dates/conversions.jl

@@ -74,8 +73,7 @@ let t = Dates.Period[Dates.Week(2), Dates.Day(14), Dates.Hour(14*24), Dates.Minu
    end
 end
 @test Dates.Year(3) == Dates.Month(36)
-@test Int(convert(Dates.Month, Dates.Year(3))) == 36
-@test Int(convert(Dates.Year, Dates.Month(36))) == 3
+@test_throws ErrorException Int(Dates.Month(36))  # eventually change to MethodError


this only warns, not throws - strangely it doesn't fail unless you run the tests with JULIA_CPU_CORES=1 though

I'll probably comment this out in #20226, but I wonder whether there's a test-system bug here.

JuliaLang#19920).

#19920). (#21861)

#19920). (#21861) (cherry picked from commit 5b33b2b)

timholy commented Jan 7, 2017

View reviewed changes

tkelman added the domain:dates Dates, times, and the Dates stdlib module label Jan 7, 2017

kshyatt added the kind:deprecation This change introduces or involves a deprecation label Jan 9, 2017

tkelman added this to the 0.6.0 milestone Jan 19, 2017

tkelman requested a review from omus January 23, 2017 19:46

omus requested changes Jan 23, 2017

View reviewed changes

omus approved these changes Jan 24, 2017

View reviewed changes

timholy force-pushed the teh/dates2 branch from 5baf826 to 2514a7b Compare January 24, 2017 17:56

tkelman reviewed Jan 24, 2017

View reviewed changes

Deprecate Dates<-->Real conversions, one(Dates), and require step i…

51b871c

…n ranges Fixes #19896

timholy force-pushed the teh/dates2 branch from 2514a7b to 51b871c Compare January 24, 2017 19:59

tkelman merged commit 8b0ca1c into master Jan 25, 2017

tkelman deleted the teh/dates2 branch January 25, 2017 05:41

tkelman mentioned this pull request Jan 25, 2017

Deprecate negate keyword argument in Dates adjuster API #20213

Merged

tkelman reviewed Jan 26, 2017

View reviewed changes

TotalVerb mentioned this pull request Jan 27, 2017

Confusing behavior when converting StepRange{<:Dates.TimeType,<:Dates.Period} to numeric range #10451

Closed

Sacha0 added a commit to Sacha0/julia that referenced this pull request May 13, 2017

Add NEWS.md entry for dep of ambiguous Dates convert and colon methods (

9ada3fc

JuliaLang#19920).

Sacha0 added the needs news A NEWS entry is required for this change label May 13, 2017

tkelman removed the needs news A NEWS entry is required for this change label May 15, 2017

tkelman pushed a commit that referenced this pull request May 15, 2017

Add NEWS.md entry for dep of ambiguous Dates convert and colon methods (

5b33b2b

#19920). (#21861)

tkelman pushed a commit that referenced this pull request May 15, 2017

Add NEWS.md entry for dep of ambiguous Dates convert and colon methods (

d13f3e9

#19920). (#21861) (cherry picked from commit 5b33b2b)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove problematic Dates conversions #19920

Remove problematic Dates conversions #19920

timholy commented Jan 7, 2017

timholy Jan 7, 2017

quinnj Jan 7, 2017

timholy Jan 7, 2017

timholy Jan 12, 2017

timholy Jan 23, 2017 •

edited

Loading

timholy Jan 24, 2017

omus Jan 24, 2017 •

edited

Loading

omus Jan 24, 2017

timholy Jan 24, 2017

quinnj Jan 24, 2017

StefanKarpinski commented Jan 8, 2017

TotalVerb commented Jan 9, 2017

tkelman commented Jan 19, 2017

timholy commented Jan 19, 2017 •

edited

Loading

omus left a comment

omus Jan 23, 2017

quinnj Jan 23, 2017

omus Jan 23, 2017

omus Jan 23, 2017

omus Jan 23, 2017

timholy Jan 23, 2017

quinnj Jan 23, 2017

omus Jan 23, 2017

timholy Jan 24, 2017

timholy commented Jan 23, 2017

StefanKarpinski commented Jan 24, 2017

omus commented Jan 24, 2017

omus commented Jan 24, 2017

timholy commented Jan 24, 2017

tkelman Jan 24, 2017

tkelman Jan 26, 2017

tkelman Jan 26, 2017

Remove problematic Dates conversions #19920

Remove problematic Dates conversions #19920

Conversation

timholy commented Jan 7, 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

timholy Jan 23, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

omus Jan 24, 2017 • edited Loading

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

StefanKarpinski commented Jan 8, 2017

TotalVerb commented Jan 9, 2017

tkelman commented Jan 19, 2017

timholy commented Jan 19, 2017 • edited Loading

omus left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timholy commented Jan 23, 2017

StefanKarpinski commented Jan 24, 2017

omus commented Jan 24, 2017

omus commented Jan 24, 2017

timholy commented Jan 24, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timholy Jan 23, 2017 •

edited

Loading

omus Jan 24, 2017 •

edited

Loading

timholy commented Jan 19, 2017 •

edited

Loading