RFC: Stop lying about eltype #280

andreasnoack · 2017-07-29T21:59:32Z

As discussed on Slack. It was actually not that bad. The core changes are in abstractdataarray.jl and defines

abstract type AbstractDataArray{T, N} <: AbstractArray{Union{T,NAtype}, N} end

Base.eltype{T, N}(d::AbstractDataArray{T, N}) = Union{T,NAtype}

So for subtypes of AbstractDataArray, the T is not referring to union but for signatures using AbstractArray the T refers to the union. This requires a little care when we ping-ping between Base and DataArrays definitions such as broadcast and similar. For that, it was convenient to define an extractT function to return the T part of a Union{T,NAtype}.

The changes here also require some changes to DataFrames. Hopefully, I'll be able to open a PR with them tomorrow and then we can look at the whole picture.

cc: @iamed2, @JeffBezanson

Update: Realized that an eltype definition is not needed here anymore since the fallback for AbstractArrays would now be correct.

nalimilan

Thanks. That's definitely more correct, though I wonder how breaking this will be for users. Do you have any ideas about that?

FWIW, this change will make DataArray very similar to Array{Union{T, Nulls.Null}. If we make this relatively deep change, maybe we should also replace NAtype with Nulls.Null?

nalimilan · 2017-07-30T14:54:51Z

src/abstractdataarray.jl

@@ -4,7 +4,7 @@
 An `N`-dimensional `AbstractArray` whose entries can take on values of type
 `T` or the value `NA`.
 """
-abstract type AbstractDataArray{T, N} <: AbstractArray{T, N} end
+abstract type AbstractDataArray{T, N} <: AbstractArray{Union{T,NAtype}, N} end


The spacing rules are getting too complex. Either use spaces after commas for all type parameters, or for none, but not just outside of unions.

nalimilan · 2017-07-30T14:59:55Z

src/broadcast.jl

+
+# This is mainly to handle isna.(x) since isna is probably the only
+# function that can guarantee that NAs will never propagate
+@inline function broadcast_t(f, ::Type{Bool}, shape, A, Bs...)


This is type piracy. I would have expected Base to do this automatically already?

No because the Base method is not extended. I just use the same name. See the comment above broadcast_c for an explanation. Base's broadcast needs some adjustments before we can use it here.

nalimilan · 2017-07-30T15:01:05Z

src/dataarray.jl

@@ -244,8 +244,6 @@ end

 dropna(dv::DataVector) = dv.data[.!dv.na] # -> Vector

-Base.broadcast(::typeof(isna), da::DataArray) = copy(da.na)


Isn't it worth keeping this specialization for performance? Remember that we have deprecated isna(da) in favor of isna.(da).

I'd prefer to clean up the code. I'm not sure how big the performance difference is but even if there is a slight difference, I'm not sure if it really matters here. Having these kinds of definitions is misleading and I wasted some time because of it since isna.(x) and (!).(isna.(x)) were returning different types.

Then at least measure the performance difference? isna is quite commonly used.

Fair call. It is indeed much faster. I'll add it back but in the broadcast.jl file instead of here.

nalimilan · 2017-07-30T15:02:46Z

src/natype.jl

@@ -36,12 +36,26 @@ struct NAException <: Exception
 end
 NAException() = NAException("NA found")

+Base.promote_rule(::Type{Union{T,NAtype}}, ::Type{Union{S,NAtype}}) where {T<:Number,S<:Number} =


I suppose these are only to fix ambiguities? If that's the case, better add a comment. If not, then why not support more general promotions? Same remark for convert.

Yes. The restriction to Number was necessary. Otherwise, it kept calling itself. I can add a comment.

~~The convert method is just to define convert in that case, not so fix an ambiguity.~~ You are probably thinking of the Number again so same reply as above

That explains why you wrote it that way, but doesn't justify why we'd want to support only Number. We should find a solution for other types. Nulls.jl only defines methods for Null (equivalent of NAtype): couldn't we do the same here?

nalimilan · 2017-07-30T15:07:49Z

src/pooleddataarray.jl

                                         pool::Vector{T},
-                                         m::AbstractArray{Bool, N},
+                                         m::Union{AbstractArray{Bool, N},


Could be written more simply as <:Union{Bool, NAtype}. You could also adopt the same approach for d and use T = extractT(eltype(d)) inside the function body.

nalimilan · 2017-07-30T15:09:29Z

src/statistics.jl

    pna = 100nacount/length(X)
    if pna != 100 # describe will fail if dropna returns an empty vector
        describe(io, dropna(X))
    else
        println(io, "Summary Stats:")
-        println(io, "Type:           $(eltype(X))")
+        println(io, "Type:           $(T)")


Why not use extractT(eltype(X)) just like below?

The type parameter is already in the signature so I think it is easier and clearer just to use it.

OK, I had misread AbstractDataArray as AbstractArray.

nalimilan · 2017-07-30T15:10:50Z

test/nas.jl

+    @testset "promotion" begin
+        @test promote_type(Int, Union{Float64,NAtype}) == Union{Float64,NAtype}
+        @test promote_type(Union{Int,NAtype}, Float64) == Union{Float64,NAtype}
+        @test promote_type(Union{Int,NAtype}, Union{Float64,NAtype}) == Union{Float64,NAtype}


Should also tests with non-number types, e.g. String.

I'm not sure if there are any promotion methods for String.

But why define methods for Number and not for other types? That was my main interrogation.

Ah, I guess you meant that it does not really make sense to promote String with anything else? Then you could try with e.g. Dates.Minute and Dates.Second.

I wanted to define if for any type but Union{Any,NAtype}==Any so that doesn't work. Basically, I don't know how we can cover all relevant cases except by adding them one by one. Number is probably the most category to cover and the only one covered by tests but I think you are right that Dates should also have a definition.

I hadn't noticed, but probably because I use my own custom promoting machinery in CSV.jl (the one place I rely on promoting, as far as I know). I need my own rules, because I want things like promote_type(::Type{Int}, ::Type{Date}) = String.

I did add the definition promote_type2{T, S}(::Type{Union{T, Null}}, ::Type{Union{S, Null}}) = Union{promote_type2(T, S), Null}; would that be enough to solve the case here?

Your last definition is what I tried first (with pomote_rule) but I got an infinite recursion out of it.

It seems to work here. Can you post exactly what you tried?

Ah, so

promote_rule(::Type{Union{T,NAtype}}, ::Type{Union{S,NAtype}}) where {T<:Dates.AbstractTime,S<:Dates.AbstractTime} = Union{promote_type(T, S),NAtype}

seems fine but

promote_rule(::Type{Union{T,NAtype}}, ::Type{S}) where {T,S} = Union{promote_type(T, S),NAtype}

causes stack overflow.

Looks like we need to define Base.promote_rule(::Type{Any}, ::Type) = Any? Maybe this should be added to Base, and shipped here temporarily with a VERSION check?

nalimilan · 2017-07-30T15:11:30Z

test/broadcast.jl

@@ -128,4 +128,9 @@
    @test map!(abs, x, x) == @data([1, 2])
    @test isequal(map!(+, DataArray(Float64, 3), @data([1, NA, 3]), @data([NA, 2, 3])), @data([NA, NA, 6]))
    @test map!(isequal, DataArray(Float64, 3), @data([1, NA, NA]), @data([1, NA, 3])) == @data([true, true, false])
+
+    # isna doesn't propagate NAs so it should return BitArrays
+    x = (!).(isna.(@data [NA, 1, 2]))


Maybe also test without (!).? The code paths used are quite different due to fusion.

andreasnoack · 2017-07-30T18:44:19Z

Thanks for the comments. I'm not sure how it will feel like in user code. We'll probably have to try it out a bit. I made the changes to DataFrames to get a feel for how much would have to be changed in library code. I don't think it was that bad and it will hopefully open up for using DataArrays in function not written explicitly for them.

My understanding is that Null is basically a reimplementation of NAtype with the main benefit that it lives in a separate package instead of being bundled with two array types. I can see that the semantics for comparisons are a bit different from NA. I could try making a commit with that change but I'm not sure it is worth bundling the two changes into the same PR.

ararslan · 2017-07-30T19:36:48Z

I could try making a commit with that change but I'm not sure it is worth bundling the two changes into the same PR.

👍 I think we should go ahead with this and try out the Nulls change in a separate branch. Luckily since the name is different (null vs NA), we should be able to put some deprecations in place that will ensure people make the right comparisons for null.

nalimilan · 2017-07-30T20:15:02Z

Sure, I didn't mean we should move to Nulls.jl in this PR, just that it could make sense to make both changes before tagging a release (if we decide to do both).

ararslan · 2017-08-14T20:04:46Z

Bump.

rofinn · 2017-08-14T20:13:02Z

src/abstractdataarray.jl

@@ -4,7 +4,7 @@
 An `N`-dimensional `AbstractArray` whose entries can take on values of type
 `T` or the value `NA`.
 """
-abstract type AbstractDataArray{T, N} <: AbstractArray{T, N} end
+abstract type AbstractDataArray{T, N} <: AbstractArray{Union{T, NAtype}, N} end


Should we maybe add an alias (e.g., const Data{T} = Union{T, NAtype}) and not export it? Reading code that operates on DataArrays.Data{T} seems more readable to me and it might help with deprecations later on.

andreasnoack · 2017-08-15T13:06:56Z

Updated. The test failure on master is most likely because of the macrocalypse going on on master right now. Hopefully, it'll either be fixed or will betold how to fix DataArrays in a couple of days.

andreasnoack · 2017-08-19T12:36:28Z

I think this is ready but right now it causes a segfault on master

rofinn · 2017-08-20T04:07:13Z

Hooray, thanks @andreasnoack!

andreasnoack force-pushed the anj/eltype branch from 9ea6e9a to 881d5ab Compare July 29, 2017 22:30

andreasnoack mentioned this pull request Jul 30, 2017

RFC: Adjust to eltype changes in DataArrays where the function now returns Union{T,NAtype} JuliaData/DataFrames.jl#1209

Closed

andreasnoack force-pushed the anj/eltype branch from 881d5ab to aab0a5b Compare July 30, 2017 02:54

nalimilan reviewed Jul 30, 2017

View reviewed changes

andreasnoack force-pushed the anj/eltype branch 2 times, most recently from bb2e425 to c70866e Compare July 30, 2017 18:32

quinnj mentioned this pull request Jul 31, 2017

Look into promotion issue JuliaData/Missings.jl#23

Closed

andreasnoack force-pushed the anj/eltype branch from c70866e to 604385d Compare August 7, 2017 14:30

ararslan approved these changes Aug 14, 2017

View reviewed changes

rofinn approved these changes Aug 14, 2017

View reviewed changes

andreasnoack added 3 commits August 18, 2017 15:03

Stop lying about eltype

500ec12

Define Data{T} = Union{T,NAtype}

1709731

Cleanup after rebase

d7897ef

andreasnoack force-pushed the anj/eltype branch from f31e879 to d7897ef Compare August 18, 2017 19:10

Fix promotion on master

b4e8218

andreasnoack changed the title ~~WIP: Stop lying about eltype~~ RFC: Stop lying about eltype Aug 19, 2017

andreasnoack merged commit 8b9e896 into master Aug 20, 2017

andreasnoack deleted the anj/eltype branch August 20, 2017 01:56

rofinn mentioned this pull request Aug 21, 2017

safe_mapslices at test/reducedim.jl is broken on 0.7 #285

Closed

nalimilan mentioned this pull request Sep 27, 2017

Port to Nulls.jl #288

Merged

piever mentioned this pull request Jan 19, 2018

sem does not accept iterables JuliaStats/StatsBase.jl#342

Closed

		@@ -244,8 +244,6 @@ end

		dropna(dv::DataVector) = dv.data[.!dv.na] # -> Vector

		Base.broadcast(::typeof(isna), da::DataArray) = copy(da.na)

RFC: Stop lying about eltype #280

RFC: Stop lying about eltype #280

Conversation

andreasnoack commented Jul 29, 2017 • edited Loading

nalimilan left a comment • edited by ararslan Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nalimilan Jul 30, 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

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

andreasnoack Jul 30, 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

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

andreasnoack Jul 31, 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

Choose a reason for hiding this comment

andreasnoack commented Jul 30, 2017

ararslan commented Jul 30, 2017

nalimilan commented Jul 30, 2017

ararslan commented Aug 14, 2017

rofinn Aug 14, 2017 • edited Loading

Choose a reason for hiding this comment

andreasnoack commented Aug 15, 2017

andreasnoack commented Aug 19, 2017

rofinn commented Aug 20, 2017

andreasnoack commented Jul 29, 2017 •

edited

Loading

nalimilan left a comment •

edited by ararslan

Loading

nalimilan Jul 30, 2017 •

edited

Loading

andreasnoack Jul 30, 2017 •

edited

Loading

andreasnoack Jul 31, 2017 •

edited

Loading

rofinn Aug 14, 2017 •

edited

Loading