Ensure levels are maintained when joining df's with Categorical cols #1266

cjprybol · 2017-10-17T19:52:22Z

coveralls · 2017-10-18T05:25:32Z

Coverage increased (+0.03%) to 72.599% when pulling ccd30da on cjp/joinlevels into 86c6145 on master.

cjprybol · 2017-10-26T20:04:07Z

Updated to include the mergelevels steps from copy! in CategoricalArrays. I think it's probably possible to refactor this to use copy! directly, but if we did that then we would need to add a few other steps that I think would ultimately make that less efficient than what we already have here. For a copy!-based approach we would need to copy, then (potentially) resize, then (potentially) promote the categorical eltype from T to Union{T, Null}, and then we'd still have to call our custom fillcolumn! function to make sure the indices from the original DataFrames get placed in the correct indices in the output DataFrame when the values in the resized column should have gaps inserted between them (to accept values from right outer join). Checking if the original columns were categorical, copying over the levels, and using mergelevels to handle the right outer join case where we need to bring in values from the right table that are missing from the left table seems like a sensible way to get the same result.

If the gaps that we insert to accept outer join values are always at the end, then I'm wrong here and copy! might allow us to get rid of fillcolumn!, but I don't think that's the case (see examples from before we fixed the ROJ bug here)

I've added a note to the docstring for join to clarify that the levels ordering of the left table takes precedence over the ordering of the right table

nalimilan

Thanks! I'd really like to avoid special-casing CategoricalArray in DataFrames, but I trust you if you say that doesn't work. Maybe we could create an API to pseudo-concatenate two arrays, something like similar but taking two arrays instead of one. But for now it will be fine.

nalimilan · 2017-10-26T20:15:40Z

test/join.jl

+        @test levels(join(B, A, on=:b, kind = :semi)[:b]) == ["a", "b", "c"]
+    end
+
+    @testset "maintain Categorical levels ordering on join - only 1 is categorical" begin


"Only LHS is categorical" (IIUC).

nalimilan · 2017-10-26T20:16:09Z

test/join.jl

@@ -325,4 +325,50 @@ module TestJoin
        @test all(isa.(o(on).columns,
                       [CategoricalVector{Union{T, Null}} for T in (Int, Float64)]))
    end
+
+    @testset "maintain Categorical levels ordering on join - non-`on` cols" begin


CategoricalArray

nalimilan · 2017-10-26T20:17:58Z

test/join.jl

+        A = DataFrame(a = [1, 2, 3], b = ["a", "b", "c"])
+        c = levels!(categorical(["a", "b", "b"]), ["b", "a"])
+        B = DataFrame(b = ["a", "b", "c"], c = c)
+        @test levels(join(A, B, on=:b, kind=:inner)[:c]) == ["b", "a"]


It would be nice to test isordered too, since that's easy to get wrong. While you're at it, testing the actual contents of the resulting column wouldn't hurt either, if that's not too much additional code.

coveralls · 2017-10-26T20:55:54Z

Coverage increased (+0.3%) to 72.819% when pulling de9a640 on cjp/joinlevels into 86c6145 on master.

coveralls · 2017-10-26T23:02:50Z

Coverage increased (+0.2%) to 72.758% when pulling 1d92fa7 on cjp/joinlevels into 86c6145 on master.

cjprybol · 2017-10-26T23:19:53Z

Ignore what I said before, a combination of permute! plus an off-by-one error I made during my first attempt at using copy! confused me into thinking something completely different was going on here. This now uses copy!.

I think I remember why we went with fillcolumn! instead of copy! the first time though. We make a copy of the original column when we subset/reorder and pass it as the source into the copy! function. One work around would be to view(col, all_orig_left/right_indices) rather than col[all_orig_left/right_indices] which would achieve the same goal and avoid the copy. We'd have to extend JuliaData/CategoricalArrays.jl#92 to accept SubArray{CategoricalArray}. Alternatively, we might be able to just permute the source column in-place (within the DataTableJoiner object).

coveralls · 2017-10-27T00:38:02Z

Coverage increased (+0.3%) to 72.819% when pulling de9a640 on cjp/joinlevels into 86c6145 on master.

nalimilan · 2017-10-27T08:58:14Z

Ah, great, that's really the most natural and generic API. Using view would also sound like the simplest solution. Can you extend JuliaData/CategoricalArrays.jl#92 to accept SubArray{CategoricalArray}?

cjprybol · 2017-11-02T00:52:23Z

This is working for me locally with JuliaData/CategoricalArrays.jl#97. We'll need to merge that, tag a new release and update this PR with a new minimum requirement on CategoricalArrays

nalimilan · 2017-11-16T22:18:26Z

CI won't pass until we move to Missings. I'm preparing a PR for that.

coveralls · 2017-11-21T00:51:38Z

Coverage decreased (-0.03%) to 72.926% when pulling c180053 on cjp/joinlevels into 2d29d2a on master.

nalimilan · 2017-11-21T13:15:17Z

Thanks! I really like how you have been able to fix a bug while removing code.

nalimilan reviewed Oct 26, 2017

View reviewed changes

cjprybol mentioned this pull request Oct 26, 2017

Expand copy! to handle nulls and view(::CategoricalArray) JuliaData/CategoricalArrays.jl#92

Closed

cjprybol force-pushed the cjp/joinlevels branch from 850574f to 9c3ed1b Compare November 2, 2017 00:44

cjprybol mentioned this pull request Nov 16, 2017

Improve performance of constructor and levels! with many levels JuliaData/CategoricalArrays.jl#100

Merged

cjprybol closed this Nov 16, 2017

cjprybol reopened this Nov 16, 2017

Use copy! in joins to preserve levels of CategoricalArray columns

c180053

cjprybol force-pushed the cjp/joinlevels branch from 9c3ed1b to c180053 Compare November 20, 2017 22:11

nalimilan approved these changes Nov 21, 2017

View reviewed changes

nalimilan merged commit ff2729b into master Nov 21, 2017

nalimilan deleted the cjp/joinlevels branch November 21, 2017 13:14

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ensure levels are maintained when joining df's with Categorical cols #1266

Ensure levels are maintained when joining df's with Categorical cols #1266

cjprybol commented Oct 17, 2017

coveralls commented Oct 18, 2017 •

edited

Loading

cjprybol commented Oct 26, 2017 •

edited

Loading

nalimilan left a comment

nalimilan Oct 26, 2017

nalimilan Oct 26, 2017

nalimilan Oct 26, 2017

coveralls commented Oct 26, 2017

coveralls commented Oct 26, 2017 •

edited

Loading

cjprybol commented Oct 26, 2017

coveralls commented Oct 27, 2017

nalimilan commented Oct 27, 2017

cjprybol commented Nov 2, 2017

nalimilan commented Nov 16, 2017

coveralls commented Nov 21, 2017 •

edited

Loading

nalimilan commented Nov 21, 2017

Ensure levels are maintained when joining df's with Categorical cols #1266

Ensure levels are maintained when joining df's with Categorical cols #1266

Conversation

cjprybol commented Oct 17, 2017

coveralls commented Oct 18, 2017 • edited Loading

cjprybol commented Oct 26, 2017 • edited Loading

nalimilan left a comment

Choose a reason for hiding this comment

nalimilan Oct 26, 2017

Choose a reason for hiding this comment

nalimilan Oct 26, 2017

Choose a reason for hiding this comment

nalimilan Oct 26, 2017

Choose a reason for hiding this comment

coveralls commented Oct 26, 2017

coveralls commented Oct 26, 2017 • edited Loading

cjprybol commented Oct 26, 2017

coveralls commented Oct 27, 2017

nalimilan commented Oct 27, 2017

cjprybol commented Nov 2, 2017

nalimilan commented Nov 16, 2017

coveralls commented Nov 21, 2017 • edited Loading

nalimilan commented Nov 21, 2017

coveralls commented Oct 18, 2017 •

edited

Loading

cjprybol commented Oct 26, 2017 •

edited

Loading

coveralls commented Oct 26, 2017 •

edited

Loading

coveralls commented Nov 21, 2017 •

edited

Loading