RFC: Fix #12074 and #12094 #12115

GordStephen · 2015-07-11T14:59:32Z

This resolves #12074 and #12094 by cleaning up and completing #12084.

Implements:

Subtraction between TimeType arrays
Addition/subtraction between Period/CompoundPeriod arrays
Addtion/subtraction between TimeType and Period/CompoundPeriod arrays
Broadcasting of scalar TimeType/Period/CompoundPeriod addition/subtraction over TimeType/Period/CompoundPeriod arrays
Tests for all of the above

…roadcasting (where not previously implemented)

tkelman · 2015-07-11T18:04:43Z

quinnj · 2015-07-13T21:51:22Z

Thanks @GordStephen for digging into this and ironing out the details here. Also thanks for the good # of tests. If you're interested in taking a crack at improving the Dates parsing code, I'm sure @jiahao would appreciate it (there should be a few open issues); I'm always happen to recruit people to work on Dates stuff.

jiahao · 2015-07-14T10:30:40Z

Always appreciate more work on Dates.

Unfortunately this PR introduced a very large number of ambiguity warnings downstream. For example:

julia> using DataFrames
Warning: New definition 
    +(AbstractArray, DataArrays.DataArray) at /Users/jiahao/.julia/v0.4/DataArrays/src/operators.jl:326
is ambiguous with: 
    +(Union{DenseArray{#P<:Union{Base.Dates.CompoundPeriod, Base.Dates.Period}, N<:Any}, Base.SubArray{#P<:Union{Base.Dates.CompoundPeriod, Base.Dates.Period}, N<:Any, A<:DenseArray, I<:Tuple{Vararg{Union{Int64, Base.Range{Int64}, Base.Colon}}}, LD<:Any}}, AbstractArray{#T<:Base.Dates.TimeType, N<:Any}) at dates/arithmetic.jl:90.
To fix, define 
    +(Union{DenseArray{#P<:Union{Base.Dates.CompoundPeriod, Base.Dates.Period}, N<:Any}, Base.SubArray{#P<:Union{Base.Dates.CompoundPeriod, Base.Dates.Period}, N<:Any, A<:DenseArray, I<:Tuple{Vararg{Union{Int64, Base.Range{Int64}, Base.Colon}}}, LD<:Any}}, DataArrays.DataArray{#T<:Base.Dates.TimeType, N<:Any})
before the new definition.
Warning: New definition 
    +(AbstractArray, DataArrays.AbstractDataArray) at /Users/jiahao/.julia/v0.4/DataArrays/src/operators.jl:349
is ambiguous with: 
    +(Union{DenseArray{#P<:Union{Base.Dates.CompoundPeriod, Base.Dates.Period}, N<:Any}, Base.SubArray{#P<:Union{Base.Dates.CompoundPeriod, Base.Dates.Period}, N<:Any, A<:DenseArray, I<:Tuple{Vararg{Union{Int64, Base.Range{Int64}, Base.Colon}}}, LD<:Any}}, AbstractArray{#T<:Base.Dates.TimeType, N<:Any}) at dates/arithmetic.jl:90.
To fix, define 
    +(Union{DenseArray{#P<:Union{Base.Dates.CompoundPeriod, Base.Dates.Period}, N<:Any}, Base.SubArray{#P<:Union{Base.Dates.CompoundPeriod, Base.Dates.Period}, N<:Any, A<:DenseArray, I<:Tuple{Vararg{Union{Int64, Base.Range{Int64}, Base.Colon}}}, LD<:Any}}, DataArrays.AbstractDataArray{#T<:Base.Dates.TimeType, N<:Any})
before the new definition.
Warning: New definition 
    -(Base.Range, DataArrays.DataArray) at /Users/jiahao/.julia/v0.4/DataArrays/src/operators.jl:326
is ambiguous with: 
    -(Base.Range{#T<:Base.Dates.TimeType}, AbstractArray{#T<:Base.Dates.TimeType, N<:Any}) at dates/arithmetic.jl:104.
To fix, define 
    -(Base.Range{#T<:Base.Dates.TimeType}, DataArrays.DataArray{#T<:Base.Dates.TimeType, N<:Any})
before the new definition.
Warning: New definition 
    -(AbstractArray, DataArrays.DataArray) at /Users/jiahao/.julia/v0.4/DataArrays/src/operators.jl:326
is ambiguous with: 
    -(Union{DenseArray{#P<:Union{Base.Dates.CompoundPeriod, Base.Dates.Period}, N<:Any}, Base.SubArray{#P<:Union{Base.Dates.CompoundPeriod, Base.Dates.Period}, N<:Any, A<:DenseArray, I<:Tuple{Vararg{Union{Int64, Base.Range{Int64}, Base.Colon}}}, LD<:Any}}, AbstractArray{#T<:Base.Dates.TimeType, N<:Any}) at dates/arithmetic.jl:90.
To fix, define 
    -(Union{DenseArray{#P<:Union{Base.Dates.CompoundPeriod, Base.Dates.Period}, N<:Any}, Base.SubArray{#P<:Union{Base.Dates.CompoundPeriod, Base.Dates.Period}, N<:Any, A<:DenseArray, I<:Tuple{Vararg{Union{Int64, Base.Range{Int64}, Base.Colon}}}, LD<:Any}}, DataArrays.DataArray{#T<:Base.Dates.TimeType, N<:Any})
before the new definition.
Warning: New definition 
    -(AbstractArray, DataArrays.DataArray) at /Users/jiahao/.julia/v0.4/DataArrays/src/operators.jl:326
is ambiguous with: 
    -(Base.Range{#T<:Base.Dates.TimeType}, AbstractArray{#T<:Base.Dates.TimeType, N<:Any}) at dates/arithmetic.jl:104.
To fix, define 
    -(Base.Range{#T<:Base.Dates.TimeType}, DataArrays.DataArray{#T<:Base.Dates.TimeType, N<:Any})
before the new definition.
Warning: New definition 
    -(AbstractArray, DataArrays.DataArray) at /Users/jiahao/.julia/v0.4/DataArrays/src/operators.jl:326
is ambiguous with: 
    -(AbstractArray{#T<:Base.Dates.TimeType, N<:Any}, AbstractArray{#T<:Base.Dates.TimeType, N<:Any}) at dates/arithmetic.jl:105.
To fix, define 
    -(AbstractArray{#T<:Base.Dates.TimeType, N<:Any}, DataArrays.DataArray{#T<:Base.Dates.TimeType, N<:Any})
before the new definition.
Warning: New definition 
    -(DataArrays.AbstractDataArray, Base.Range) at /Users/jiahao/.julia/v0.4/DataArrays/src/operators.jl:349
is ambiguous with: 
    -(AbstractArray{#T<:Base.Dates.TimeType, N<:Any}, Base.Range{#T<:Base.Dates.TimeType}) at dates/arithmetic.jl:103.
To fix, define 
    -(DataArrays.AbstractDataArray{#T<:Base.Dates.TimeType, N<:Any}, Base.Range{#T<:Base.Dates.TimeType})
before the new definition.
Warning: New definition 
    -(Base.Range, DataArrays.AbstractDataArray) at /Users/jiahao/.julia/v0.4/DataArrays/src/operators.jl:349
is ambiguous with: 
    -(Base.Range{#T<:Base.Dates.TimeType}, AbstractArray{#T<:Base.Dates.TimeType, N<:Any}) at dates/arithmetic.jl:104.
To fix, define 
    -(Base.Range{#T<:Base.Dates.TimeType}, DataArrays.AbstractDataArray{#T<:Base.Dates.TimeType, N<:Any})
before the new definition.
Warning: New definition 
    -(DataArrays.AbstractDataArray, AbstractArray) at /Users/jiahao/.julia/v0.4/DataArrays/src/operators.jl:349
is ambiguous with: 
    -(AbstractArray{#T<:Base.Dates.TimeType, N<:Any}, Base.Range{#T<:Base.Dates.TimeType}) at dates/arithmetic.jl:103.
To fix, define 
    -(DataArrays.AbstractDataArray{#T<:Base.Dates.TimeType, N<:Any}, Base.Range{#T<:Base.Dates.TimeType})
before the new definition.
Warning: New definition 
    -(DataArrays.AbstractDataArray, AbstractArray) at /Users/jiahao/.julia/v0.4/DataArrays/src/operators.jl:349
is ambiguous with: 
    -(AbstractArray{#T<:Base.Dates.TimeType, N<:Any}, Union{DenseArray{#P<:Union{Base.Dates.CompoundPeriod, Base.Dates.Period}, N<:Any}, Base.SubArray{#P<:Union{Base.Dates.CompoundPeriod, Base.Dates.Period}, N<:Any, A<:DenseArray, I<:Tuple{Vararg{Union{Int64, Base.Range{Int64}, Base.Colon}}}, LD<:Any}}) at dates/arithmetic.jl:88.
To fix, define 
    -(DataArrays.AbstractDataArray{#T<:Base.Dates.TimeType, N<:Any}, Union{DenseArray{#P<:Union{Base.Dates.CompoundPeriod, Base.Dates.Period}, N<:Any}, Base.SubArray{#P<:Union{Base.Dates.CompoundPeriod, Base.Dates.Period}, N<:Any, A<:DenseArray, I<:Tuple{Vararg{Union{Int64, Base.Range{Int64}, Base.Colon}}}, LD<:Any}})
before the new definition.
Warning: New definition 
    -(DataArrays.AbstractDataArray, AbstractArray) at /Users/jiahao/.julia/v0.4/DataArrays/src/operators.jl:349
is ambiguous with: 
    -(AbstractArray{#T<:Base.Dates.TimeType, N<:Any}, AbstractArray{#T<:Base.Dates.TimeType, N<:Any}) at dates/arithmetic.jl:105.
To fix, define 
    -(DataArrays.AbstractDataArray{#T<:Base.Dates.TimeType, N<:Any}, AbstractArray{#T<:Base.Dates.TimeType, N<:Any})
before the new definition.
Warning: New definition 
    -(AbstractArray, DataArrays.AbstractDataArray) at /Users/jiahao/.julia/v0.4/DataArrays/src/operators.jl:349
is ambiguous with: 
    -(Union{DenseArray{#P<:Union{Base.Dates.CompoundPeriod, Base.Dates.Period}, N<:Any}, Base.SubArray{#P<:Union{Base.Dates.CompoundPeriod, Base.Dates.Period}, N<:Any, A<:DenseArray, I<:Tuple{Vararg{Union{Int64, Base.Range{Int64}, Base.Colon}}}, LD<:Any}}, AbstractArray{#T<:Base.Dates.TimeType, N<:Any}) at dates/arithmetic.jl:90.
To fix, define 
    -(Union{DenseArray{#P<:Union{Base.Dates.CompoundPeriod, Base.Dates.Period}, N<:Any}, Base.SubArray{#P<:Union{Base.Dates.CompoundPeriod, Base.Dates.Period}, N<:Any, A<:DenseArray, I<:Tuple{Vararg{Union{Int64, Base.Range{Int64}, Base.Colon}}}, LD<:Any}}, DataArrays.AbstractDataArray{#T<:Base.Dates.TimeType, N<:Any})
before the new definition.
Warning: New definition 
    -(AbstractArray, DataArrays.AbstractDataArray) at /Users/jiahao/.julia/v0.4/DataArrays/src/operators.jl:349
is ambiguous with: 
    -(Base.Range{#T<:Base.Dates.TimeType}, AbstractArray{#T<:Base.Dates.TimeType, N<:Any}) at dates/arithmetic.jl:104.
To fix, define 
    -(Base.Range{#T<:Base.Dates.TimeType}, DataArrays.AbstractDataArray{#T<:Base.Dates.TimeType, N<:Any})
before the new definition.
Warning: New definition 
    -(AbstractArray, DataArrays.AbstractDataArray) at /Users/jiahao/.julia/v0.4/DataArrays/src/operators.jl:349
is ambiguous with: 
    -(AbstractArray{#T<:Base.Dates.TimeType, N<:Any}, AbstractArray{#T<:Base.Dates.TimeType, N<:Any}) at dates/arithmetic.jl:105.
To fix, define 
    -(AbstractArray{#T<:Base.Dates.TimeType, N<:Any}, DataArrays.AbstractDataArray{#T<:Base.Dates.TimeType, N<:Any})
before the new definition.

GordStephen · 2015-07-14T11:23:03Z

Yikes! Well, I guess the good news is that the errors all seem to trace back to just two method definitions (lines 326/349 in DataArrays/src/operators.jl). I'll go open an issue at DataArrays and see what I can do about it - or do you think there's something that could be done on this end to reduce the impact more generally?

timholy · 2015-07-18T19:16:12Z

The ~~deprecations~~ambiguities also affect Images.

I wonder if it's worth at least a little time seeing if we can fix this more generally. Assuming you don't want to make a Dates.TimeType into a subtype of Number, I wonder if we should introduce an AbstractScalar type? And then we could generalize the definitions in arraymath.jl, delete the time-specific methods, and then package authors only have one place they have to worry about in terms of ambiguities.

GordStephen · 2015-07-20T16:53:36Z

I like the sound of that - one thing to watch for would be the fact that TimeType/Period/CompoundPeriod operations often produce results that are typed differently than their operands, which I've found to be tricky to deal with very generally. But a general solution would definitely be preferable - right now defining date array methods is like fighting some sort of hydra - fixing one ambiguity usually creates a bunch more!

timholy · 2015-07-23T17:34:03Z

After working on this for a few hours, I don't think we can have a generic fix without addressing #8027. The specific place this comes up in Dates is in adding a CompoundPeriod to a Period, which involves concatenation and hence should promote two Periods to Period; however, falling back on generic AbstractArray operations for A+b leads one to wish for a promotion rule that selects CompoundPeriod.

Now here's my provocative question: is the current slew of ambiguity warnings in packages worth the benefit of this PR? Or should we revert it?

GordStephen · 2015-07-24T14:51:20Z

@timholy, thanks for all the work to try to fix the underlying issues here! Maybe the solution for now is to move the functionality into a standalone module, so the few who need it right now (maybe just me?) can have it without causing broader package issues.

timholy · 2015-07-24T15:41:31Z

Let's hold out for a bit more and see whether #12292 wraps up soon. Certainly it would be great to support the operations you've supported.

If the answer on #12292 is "just can't fix this now," I confess I like your solution. I hate the idea of backing out excellent work, especially from a new contributor. But to me it seems like the only workable choice. Your flexibility gains a big 💯 from me, and I hope to see you around more!

GordStephen · 2015-07-26T23:07:51Z

Ok, sounds good!

Add Period/CompoundPeriod/TimeType array addition, subtraction, and b…

bed9d29

…roadcasting (where not previously implemented)

GordStephen mentioned this pull request Jul 11, 2015

WIP: Address #12074 - Date array operations #12084

Closed

quinnj merged commit bed9d29 into JuliaLang:master Jul 13, 2015

quinnj mentioned this pull request Jul 13, 2015

Incorrect broadcasted Period subtraction behaviour? #12094

Closed

GordStephen mentioned this pull request Jul 14, 2015

Base.Dates ambiguity errors for array addition/subtraction JuliaStats/DataArrays.jl#158

Closed

timholy mentioned this pull request Jul 21, 2015

Warnings related to package precompilation #12252

Closed

This was referenced Jul 23, 2015

Arithmetic on types? #8027

Closed

Enable operator-sensitive extension of element-type promotion #12292

Merged

timholy mentioned this pull request Jul 29, 2015

Handle Dates-array arithmetic with promote_op #12370

Merged

This was referenced Jan 25, 2017

Deprecate nonsensical commutative Dates subtraction methods #20205

Closed

Various Date/Time fixes #20226

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

RFC: Fix #12074 and #12094 #12115

RFC: Fix #12074 and #12094 #12115

GordStephen commented Jul 11, 2015

tkelman commented Jul 11, 2015

quinnj commented Jul 13, 2015

jiahao commented Jul 14, 2015

GordStephen commented Jul 14, 2015

timholy commented Jul 18, 2015

GordStephen commented Jul 20, 2015

timholy commented Jul 23, 2015

GordStephen commented Jul 24, 2015

timholy commented Jul 24, 2015

GordStephen commented Jul 26, 2015

RFC: Fix #12074 and #12094 #12115

RFC: Fix #12074 and #12094 #12115

Conversation

GordStephen commented Jul 11, 2015

tkelman commented Jul 11, 2015

quinnj commented Jul 13, 2015

jiahao commented Jul 14, 2015

GordStephen commented Jul 14, 2015

timholy commented Jul 18, 2015

GordStephen commented Jul 20, 2015

timholy commented Jul 23, 2015

GordStephen commented Jul 24, 2015

timholy commented Jul 24, 2015

GordStephen commented Jul 26, 2015