Stack should use similar_nullable, not NullableArray #54

cjprybol · 2017-04-13T00:20:41Z

Adds a test set to trigger multiple entries in unstack warning
Adds a similar_nullable function for NullableCategoricalArrays
Tests to make sure column types are converted to their appropriate nullable type.

ararslan · 2017-04-13T00:55:11Z

src/abstractdatatable/reshape.jl

@@ -98,8 +98,8 @@ function stack(dt::AbstractDataTable, measure_vars::Vector{Int}, id_var::Int;
 end
 function stack(dt::AbstractDataTable, measure_var::Int, id_vars::Vector{Int};
               variable_name::Symbol=:variable, value_name::Symbol=:value)
-    stackdt(dt, [measure_var], id_vars;
-            variable_name=variable_name, value_name=value_name)
+    stack(dt, [measure_var], id_vars;


Is this change related?

Local tests say no, so it must be something I hadn't yet reverted. I'm not sure why this is the only stack function calling stackdt, but I'll investigate that when I update the indexing of stackdt's custom array views.

The commit message shouldn't mention this change now that it's not included.

nalimilan

Thanks! Could you add a test for the incorrect type that you fixed?

nalimilan · 2017-04-15T14:12:35Z

test/datatable.jl

@@ -341,4 +341,21 @@ module TestDataTable
    @test find(c -> isa(c, NullableCategoricalArray), categorical!(DataTable(A=1:3, B=4:6), :A).columns) == [1]
    @test find(c -> isa(c, NullableCategoricalArray), categorical!(DataTable(A=1:3, B=4:6), [1]).columns) == [1]
    @test find(c -> isa(c, NullableCategoricalArray), categorical!(DataTable(A=1:3, B=4:6), 1).columns) == [1]
+
+    @testset "multiple entries in unstack warnings" begin


This is great but should go to a separate commit since it's not related to the above changes AFAICT.

cjprybol · 2017-04-16T20:56:18Z

Updated. I tagged on a commit at the end to fix the deprecation warnings for similar_nullable as the new tests triggered the depwarn.

nalimilan

To be clear, this exercizes the code path, but doesn't test that warnings are thrown, right? The commit message could explain this. I don't know how to catch warnings, but ideally we would check that they are thrown as expected.

nalimilan · 2017-04-16T21:31:05Z

src/abstractdatatable/join.jl

@@ -15,6 +15,9 @@ similar_nullable{T,R}(dv::CategoricalArray{T,R}, dims::@compat(Union{Int, Tuple{
 similar_nullable(dt::AbstractDataTable, dims::Int) =
    DataTable(Any[similar_nullable(x, dims) for x in columns(dt)], copy(index(dt)))

+similar_nullable{T,R}(dv::NullableCategoricalArray{T,R}, dims::Union{Int, Tuple{Vararg{Int}}}) =


Would be more logical to add this definition right after that for CategoricalArray.

The AbstractDataTable function that separates the two will disappear in #44. I thought adding the new function to the end of the set like this would minimize the risk of a merge conflict, although that's purely speculative. I can reverse them if you'd like, but I'm not sure it will matter.

Not a big deal, but it's always better to have commits be correct when taken in isolation.

nalimilan · 2017-04-16T21:33:05Z

test/datatable.jl

+        udt = unstack(long, :variable, :value)
+        @test udt == DataTable(Any[collect(1:12), repeat(1:3, inner = 4),
+                                   repeat(1:4, inner = 3),
+                                   Nullable{Float64}.(linspace(0, 1, 12)),


Just Nullable. is enough. Or even NullableArray since that's a more common structure to use.

nalimilan · 2017-04-16T21:33:58Z

test/datatable.jl

+    @testset "unstack nullable promotion" begin
+        long = DataTable(Any[repeat([:c, :d], inner=12),
+                             vcat(linspace(0, 1, 12), linspace(2, 1, 12)),
+                             repeat(1:12, outer=2), repmat(repeat(1:3, inner=4), 2),


(Only applies if you keep this.) Simply repeat(1:3, inner=4, outer=2). Same below.

nalimilan · 2017-04-16T21:35:44Z

test/datatable.jl

@@ -342,6 +342,40 @@ module TestDataTable
    @test find(c -> isa(c, NullableCategoricalArray), categorical!(DataTable(A=1:3, B=4:6), [1]).columns) == [1]
    @test find(c -> isa(c, NullableCategoricalArray), categorical!(DataTable(A=1:3, B=4:6), 1).columns) == [1]

+    @testset "unstack nullable promotion" begin
+        long = DataTable(Any[repeat([:c, :d], inner=12),
+                             vcat(linspace(0, 1, 12), linspace(2, 1, 12)),


Can't you find a simpler example which doesn't involve repeat nor linspace? Since the goal here is to test promotion, focus on testing types rather than contents. BTW, types are not tested.

I took this example from the unstack docstring, replacing rand with linspace to avoid srand and random number issues. I've updated it a bit to make it a little simpler. The two tested types here are converting standard arrays to NullableArrays and CategoricalArrays to NullableCategoricalArrays, with the latter being the primary issue of the current master implementation. Other types are tested in assorted places where other calls to unstack are made.

I took this example from the unstack docstring, replacing rand with linspace to avoid srand and random number issues.

Yes, but there's no reason to use such a complicated example here. Just a handful a values are enough to test that promotion works.

The two tested types here are converting standard arrays to NullableArrays and CategoricalArrays to NullableCategoricalArrays,

But you should explicitly test for column types of the result. The current code only checks that it compares equal, which can happen with any nullable vector, not just NullableCategoricalArray (and in the future, maybe even with a non-nullable vector with the new Nullable implementation).

nalimilan · 2017-04-16T21:36:26Z

test/datatable.jl

+                                   NullableCategoricalArray(linspace(2, 1, 12))],
+                               [:id, :a, :b, :c, :d])
+        @test unstack(long) ==
+          unstack(long, :id, :variable, :value) ==


Incorrect indentation.

nalimilan · 2017-04-16T21:38:54Z

test/datatable.jl

+        dt = DataTable(id=[1, 2, 1, 2], variable=["a", "b", "a", "b"], value=[3, 4, 5, 6])
+        a = unstack(dt, :id, :variable, :value)
+        b = unstack(dt, :variable, :value)
+        @test a == b == DataTable(id = Nullable[1, 2], a = Nullable[5, Nullable()], b =  Nullable[Nullable(), 6])


Nullable[ isn't needed for the two last uses and is actually even counter-productive here since the array will have an abstract element type. Elsewhere, use Nullable{Int}.

ararslan · 2017-04-17T06:00:38Z

On 0.6 you can test warnings with @test_warn (IIRC). That may have been ported to BaseTestNext, in which case you could use that.

cjprybol · 2017-04-17T06:39:52Z

Thanks for the suggestion, Alex! Didn't know about @test_warn and @test_nowarn, but I'm glad they are in 0.6! I've updated the tests to check if those macros are defined and if so, the warnings are checked. Let's see if the CI tests for 0.5 complain or if that will work.

I couldn't find anything in BaseTestNext or compat to extend supporting those macros into v0.5

nalimilan · 2017-04-17T09:17:35Z

test/datatable.jl

+
+    @testset "duplicate entries in unstack warnings" begin
+        dt = DataTable(id=NullableArray([1, 2, 1, 2]), variable=["a", "b", "a", "b"], value=[3, 4, 5, 6])
+        if isdefined(Symbol(:@test_warn))


Should be isdefined(Base.Test, Symbol("@test_warn")). This is why the tests failed.

I couldn't get isdefined to work because Julia will still try and expand the @test_warn macro during parsing. I think it needs a @static check to avoid the if block entirely during parsing in v0.5. A static check against the first tagged version with the @test_warn macro works, although I'm not sure how to reference the exact commit. JuliaLang/julia@28a11ff

Run ./contrib/commit-name.sh 28a11ffd4d2e8451492013f18dad5e9a576233f6 from the root directory of a clone of Julia to get the exact version number for a particular commit.

To get around the eager macro expansion,

if isdefined(Base.Test, Symbol("@test_warn")) include_string(""" @test_warn ... """) end

Just bookmarked both of those comments.

In v0.6, where the macros test_warn and test_nowarn are defined, tests will actually check the warning messages. In v0.5 warnings are not checked but can be manually confirmed by reviewing the output of running Pkg.test("DataTables").

nalimilan · 2017-04-17T20:01:37Z

src/abstractdatatable/reshape.jl

@@ -235,15 +232,10 @@ function unstack(dt::AbstractDataTable, colkey::Int, value::Int)
    end
    keycol = NullableCategoricalArray(dt[colkey])
    valuecol = dt[value]
-    dt1 = dt[g.idx[g.starts], g.cols]
+    dt1 = nullable!(dt[g.idx[g.starts], g.cols], g.cols)


No need to specify g.cols the second time AFAICT.

The nullify! function I had written for the pre-refactor PR didn't need any specification of columns when you want to nullify all of the columns, but this existing nullable! function does require columns to be specified.

julia> nullable!(DataTable(a = 1:4)) ERROR: MethodError: no method matching nullable!(::DataTables.DataTable) Closest candidates are: nullable!(::DataTables.DataTable, ::Union{Real, Symbol}) at /Users/Cameron/.julia/v0.6/DataTables/src/datatable/datatable.jl:779 nullable!(::DataTables.DataTable, ::Array{T<:Union{Real, Symbol},1}) where T<:Union{Real, Symbol} at /Users/Cameron/.julia/v0.6/DataTables/src/datatable/datatable.jl:783 nullable!(::Array{Symbol,1}, ::DataTables.AbstractDataTable) at deprecated.jl:50 ... julia> nullable!(DataTable(a = 1:4), :a) 4×1 DataTables.DataTable │ Row │ a │ ├─────┼───┤ │ 1 │ 1 │ │ 2 │ 2 │ │ 3 │ 3 │ │ 4 │ 4 │

OK, so that's fine for now.

nalimilan · 2017-04-17T20:03:52Z

test/datatable.jl

+                       [:id, :variable, :value])
+        udt = unstack(dt)
+        @test udt == unstack(dt, :variable, :value) == unstack(dt, :id, :variable, :value)
+        @test udt == DataTable(Any[NullableCategoricalArray([1, 2]),


Nullable[1, 2] and so on is enough here, NullableCategoricalArray is only needed in the type check.

Stack was using NullableArray, which would incorrectly cast CategoricalArrays to NullableArrays{CategoricalValue}. Adds a new similar_nullable function for NullableCategoricalArrays.

cjprybol mentioned this pull request Apr 13, 2017

Stop auto-promoting column-types #30

Closed

ararslan reviewed Apr 13, 2017

View reviewed changes

nalimilan reviewed Apr 15, 2017

View reviewed changes

nalimilan reviewed Apr 16, 2017

View reviewed changes

nalimilan reviewed Apr 17, 2017

View reviewed changes

cjprybol added 2 commits April 17, 2017 13:18

Update stack functions to use similar_nullable

71edd51

Stack was using NullableArray, which would incorrectly cast CategoricalArrays to NullableArrays{CategoricalValue}. Adds a new similar_nullable function for NullableCategoricalArrays.

Update similar_nullable conventions (depwarn and compat)

7b59074

nalimilan approved these changes Apr 23, 2017

View reviewed changes

ararslan merged commit 817ab76 into JuliaData:master Apr 23, 2017

cjprybol deleted the cjp/stacktypes branch April 24, 2017 06:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Stack should use similar_nullable, not NullableArray #54

Stack should use similar_nullable, not NullableArray #54

cjprybol commented Apr 13, 2017 •

edited

ararslan Apr 13, 2017

cjprybol Apr 13, 2017

nalimilan Apr 15, 2017

nalimilan left a comment

nalimilan Apr 15, 2017

cjprybol commented Apr 16, 2017

nalimilan left a comment

nalimilan Apr 16, 2017

cjprybol Apr 16, 2017

nalimilan Apr 17, 2017

nalimilan Apr 16, 2017

nalimilan Apr 16, 2017

nalimilan Apr 16, 2017

cjprybol Apr 17, 2017

nalimilan Apr 17, 2017

cjprybol Apr 17, 2017

nalimilan Apr 16, 2017

nalimilan Apr 16, 2017

ararslan commented Apr 17, 2017

cjprybol commented Apr 17, 2017

nalimilan Apr 17, 2017

cjprybol Apr 17, 2017

ararslan Apr 17, 2017 •

edited

ararslan Apr 17, 2017

cjprybol Apr 17, 2017

nalimilan Apr 17, 2017

cjprybol Apr 17, 2017

nalimilan Apr 17, 2017

nalimilan Apr 17, 2017

Stack should use similar_nullable, not NullableArray #54

Stack should use similar_nullable, not NullableArray #54

Conversation

cjprybol commented Apr 13, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nalimilan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cjprybol commented Apr 16, 2017

nalimilan 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ararslan commented Apr 17, 2017

cjprybol commented Apr 17, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ararslan Apr 17, 2017 • edited

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

cjprybol commented Apr 13, 2017 •

edited

ararslan Apr 17, 2017 •

edited