Add missing promote_op method for Nullable

[omus]

I noticed that these promotion methods were missing for Nullable types when working with the following code:

julia> using NullableArrays

julia> NullableArray([DateTime(2016)]) - NullableArray([DateTime(2015)])
ERROR: MethodError: Cannot `convert` an object of type Base.Dates.Millisecond to an object of type DateTime
This may have arisen from a call to the constructor DateTime(...),
since type constructors fall back to convert methods.
Closest candidates are:
  convert(::Type{DateTime}, ::Date)
  convert{R<:Real}(::Type{DateTime}, ::R<:Real)
  convert{T}(::Type{T}, ::T)
  ...
 [inlined code] from ./nullable.jl:40
 in -(::NullableArrays.NullableArray{DateTime,1}, ::NullableArrays.NullableArray{DateTime,1}) at ./arraymath.jl:85
 in eval(::Module, ::Any) at ./boot.jl:236

These changes should be backported to 0.4. The promote_op test will need to be updated to work on 0.4 and 0.5.

[yuyichao]

#15892 ?

[omus]

Thanks @yuyichao I missed that one. That PR is missing the promote_op method I added which means this PR isn't a duplicate.

[omus]

@yuyichao I believe that this should be reopened. There is overlap with #15892 but they are not duplicates of each other.

Previous Julia behaviour for promote_op in 0.5:

julia> Base.promote_op(-, DateTime, DateTime)
Base.Dates.Millisecond

julia> Base.promote_op(-, Nullable{DateTime}, Nullable{DateTime})
Nullable{DateTime}

PR Julia behaviour in 0.5:

julia> Base.promote_op(-, DateTime, DateTime)
Base.Dates.Millisecond

julia> Base.promote_op(-, Nullable{DateTime}, Nullable{DateTime})
Nullable{Base.Dates.Millisecond}

[yuyichao]

It is still unclear if doing this is a good idea at all. (Also, I'm fine with leaving this open but, github doesn't want to reopen this due to "the branch was force-pused or recreated")

[yuyichao]

In another word, feel free to open a new one agains master or that branch. The discussion of whether this should be done should be continue there....

[omus]

@yuyichao sorry that was my fault. You should be able to re-open the PR now.

[nalimilan]

The first part is indeed #15892, and I think it should be merged.

The second one (promote_op) is probably more controversial, though I also support it. The question is how far should we go towards "auto-lifting". See in particular JuliaStats/NullableArrays.jl#20, JuliaStats/NullableArrays.jl#95. I've come to the conclusion that we should support lifting for basic operations (like C#), and only require using an explicit macro when calling functions. This is what this PR does.

Cc: @johnmyleswhite @DavidGold @tshort

[omus]

I've rebased the changes. Since the other promotion rules were merged this PR is now only about the promote_op addition.

[nalimilan]

promote_type is tested above now, so you could as well remove the loop and simply copy the line for a few types. Would make sense to put it right below the promote_type block.

Also, it would be good to test the actual behaviour, i.e addition, subtraction, etc. of nullables.

[omus]

Ok, I'll update this. What operations should I be testing? Just addition and subtraction?

[nalimilan]

Actually, I guess you should test all of them in a loop similar to the one used to define the methods. Else some code won't be covered at all.

[nalimilan]

Actually testing arithmetic operations isn't possible in Base of course until we define arithmetic operations there. Could you simply apply the changes I suggested in my first comment then? I think we can merge this soon since we all agree that it's needed.

[nalimilan]

+1 to this, but I think we need to actually move even more code from NullableArrays into Base. Indeed, if you don't call using NullableArrays, your example doesn't work with Array{Nullable} (i.e. Nullable[DateTime(2016)] - Nullable[DateTime(2015)]), which also uses promote_op, because the operations on Nullable aren't defined.

I think the consensus is to support lifting behavior for all operators, like C#. It can easily be done by iterating over names(Base.operators) and defining methods for these which call the corresponding operator on the values.

@johnmyleswhite Do you support this too? Note that it does not introduce operations between scalars and Nullable. So AFAICT it's equivalent to the C# behavior. That would allow removing some more code from NullableArrays which doesn't belong there.

[johnmyleswhite]

I'm generally in favor of introducing these promotions, but we need to be very favor in how we define things like +: the current approach substantially favors performance over safety and will not work well if people start using types that aren't bitstypes.

[omus]

@nalimilan I've implemented the promote_op tests for a couple of types. I think this PR is ready to merge.

[nalimilan]

Since it's used only once and isn't related to nullables in general, better write Base.Dates.Milliseconds once below instead of importing.

[omus]

Ok, will do. I was trying to keep the line under 92 characters.

[nalimilan]

I see. Adding a line break should be OK too. Anyway, not a big deal either way.

[omus]

I think it reads best if it is all on one line. I'll remove the import and just go over the 92 character guideline.

[nalimilan]

Thanks! See JuliaStats/NullableArrays.jl#111 if you want to help on actually implementing these operators in Base.