From 5528a9abc2f55cb6b10f15168cc198791fd2147d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bogumi=C5=82=20Kami=C5=84ski?= Date: Thu, 22 Nov 2018 18:33:36 +0100 Subject: [PATCH 1/9] copyto! and append! reference implementation --- src/array.jl | 91 ++++++++++++++++++++++------------------------------ 1 file changed, 39 insertions(+), 52 deletions(-) diff --git a/src/array.jl b/src/array.jl index d6c3e5e5..c2eb45df 100644 --- a/src/array.jl +++ b/src/array.jl @@ -405,66 +405,40 @@ copy(A::CategoricalArray) = deepcopy(A) CatArrOrSub{T, N} = Union{CategoricalArray{T, N}, SubArray{<:Any, N, <:CategoricalArray{T}}} where {T, N} -function copyto!(dest::CatArrOrSub{T, N}, dstart::Integer, - src::CatArrOrSub{<:Any, N}, sstart::Integer, - n::Integer) where {T, N} +function copyto!(dest::CatArrOrSub{T}, dstart::Integer, + src::AbstractArray, sstart::Integer, + n::Integer) where {T} n == 0 && return dest n < 0 && throw(ArgumentError(string("tried to copy n=", n, " elements, but n should be nonnegative"))) destinds, srcinds = LinearIndices(dest), LinearIndices(src) (dstart ∈ destinds && dstart+n-1 ∈ destinds) || throw(BoundsError(dest, dstart:dstart+n-1)) (sstart ∈ srcinds && sstart+n-1 ∈ srcinds) || throw(BoundsError(src, sstart:sstart+n-1)) - drefs = refs(dest) - srefs = refs(src) - dpool = pool(dest) - spool = pool(src) - - # try converting src to dest type to avoid partial copy corruption of dest - # in the event that the src cannot be copied into dest - slevs = convert(Vector{T}, levels(src)) - if eltype(src) >: Missing && !(eltype(dest) >: Missing) && !all(x -> x > 0, srefs) - throw(MissingException("cannot copy array with missing values to an array with element type $T")) - end - - newlevels, ordered = mergelevels(isordered(dest), levels(dest), slevs) - # Orderedness cannot be preserved if the source was unordered and new levels - # need to be added: new comparisons would only be based on the source's order - # (this is consistent with what happens when adding a new level via setindex!) - ordered &= isordered(src) | (length(newlevels) == length(levels(dest))) - if ordered != isordered(dest) - isa(dest, SubArray) && throw(ArgumentError("cannot set ordered=$ordered on dest SubArray as it would affect the parent. Found when trying to set levels to $newlevels.")) - ordered!(dest, ordered) - end - - # Simple case: replace all values - if !isa(dest, SubArray) && dstart == dstart == 1 && n == length(dest) == length(src) - # Set index to reflect refs - levels!(dpool, T[]) # Needed in case src and dest share some levels - levels!(dpool, index(spool)) - - # Set final levels in their visible order - levels!(dpool, newlevels) - - copyto!(drefs, srefs) - else # More work to do: preserve some values (and therefore index) - levels!(dpool, newlevels) - - indexmap = indexin(index(spool), index(dpool)) - - @inbounds for i = 0:(n-1) - x = srefs[sstart+i] - drefs[dstart+i] = x > 0 ? indexmap[x] : 0 + if isordered(dest) + slevelsdest = Set(levels(dest)) + if eltype(dest) >: Missing + push!(slevesdest, missing) + end + if !all(x -> x in slevesdest, view(src, sstart:sstart+n-1)) + throw(ArgumentError("src in range $(sstart):$(sstart+n+1) contains values not allowed in dest which is ordered")) end + else + if eltype(src) >: Missing && !(eltype(dest) >: Missing) && any(ismissing, view(src, sstart:sstart+n-1)) + throw(MissingException("cannot copy array with missing values to an array with element type $T")) + end + end + for i in 1:n + dst[i] = src[i] end dest end -copyto!(dest::CatArrOrSub, src::CatArrOrSub) = +copyto!(dest::CatArrOrSub, src::AbstractArray) = copyto!(dest, 1, src, 1, length(src)) -copyto!(dest::CatArrOrSub, dstart::Integer, src::CatArrOrSub) = +copyto!(dest::CatArrOrSub, dstart::Integer, src::AbstractArray) = copyto!(dest, dstart, src, 1, length(src)) @static if VERSION >= v"0.7.0-DEV.3208" @@ -694,15 +668,28 @@ function Base.push!(A::CategoricalVector, item) A end -function Base.append!(A::CategoricalVector, B::CatArrOrSub) - levels!(A, union(levels(A), levels(B))) - len = length(A) - len2 = length(B) - resize!(A.refs, len + len2) +function Base.append!(dest::CategoricalVector, src::AbstractArray) + if isordered(dest) + slevelsdest = Set(levels(dest)) + if eltype(dest) >: Missing + push!(slevesdest, missing) + end + if !all(x -> x in slevesdest, src) + throw(ArgumentError("src contains values not allowed in dest which is ordered")) + end + else + if eltype(src) >: Missing && !(eltype(dest) >: Missing) && any(ismissing, src) + throw(MissingException("cannot append array with missing values to an array with element type $T")) + end + end + + len = length(dest) + len2 = length(src) + resize!(dest.refs, len + len2) for i = 1:len2 - A[len + i] = B[i] + dest[len + i] = src[i] end - return A + return dest end Base.empty!(A::CategoricalArray) = (empty!(A.refs); return A) From 5b5f56569aab0ac35108ee7df30a96fe60f13eec Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bogumi=C5=82=20Kami=C5=84ski?= Date: Mon, 26 Nov 2018 08:55:26 +0100 Subject: [PATCH 2/9] Revert "copyto! and append! reference implementation" This reverts commit 5528a9abc2f55cb6b10f15168cc198791fd2147d. --- src/array.jl | 91 ++++++++++++++++++++++++++++++---------------------- 1 file changed, 52 insertions(+), 39 deletions(-) diff --git a/src/array.jl b/src/array.jl index c2eb45df..d6c3e5e5 100644 --- a/src/array.jl +++ b/src/array.jl @@ -405,40 +405,66 @@ copy(A::CategoricalArray) = deepcopy(A) CatArrOrSub{T, N} = Union{CategoricalArray{T, N}, SubArray{<:Any, N, <:CategoricalArray{T}}} where {T, N} -function copyto!(dest::CatArrOrSub{T}, dstart::Integer, - src::AbstractArray, sstart::Integer, - n::Integer) where {T} +function copyto!(dest::CatArrOrSub{T, N}, dstart::Integer, + src::CatArrOrSub{<:Any, N}, sstart::Integer, + n::Integer) where {T, N} n == 0 && return dest n < 0 && throw(ArgumentError(string("tried to copy n=", n, " elements, but n should be nonnegative"))) destinds, srcinds = LinearIndices(dest), LinearIndices(src) (dstart ∈ destinds && dstart+n-1 ∈ destinds) || throw(BoundsError(dest, dstart:dstart+n-1)) (sstart ∈ srcinds && sstart+n-1 ∈ srcinds) || throw(BoundsError(src, sstart:sstart+n-1)) - if isordered(dest) - slevelsdest = Set(levels(dest)) - if eltype(dest) >: Missing - push!(slevesdest, missing) - end - if !all(x -> x in slevesdest, view(src, sstart:sstart+n-1)) - throw(ArgumentError("src in range $(sstart):$(sstart+n+1) contains values not allowed in dest which is ordered")) - end - else - if eltype(src) >: Missing && !(eltype(dest) >: Missing) && any(ismissing, view(src, sstart:sstart+n-1)) - throw(MissingException("cannot copy array with missing values to an array with element type $T")) - end + drefs = refs(dest) + srefs = refs(src) + dpool = pool(dest) + spool = pool(src) + + # try converting src to dest type to avoid partial copy corruption of dest + # in the event that the src cannot be copied into dest + slevs = convert(Vector{T}, levels(src)) + if eltype(src) >: Missing && !(eltype(dest) >: Missing) && !all(x -> x > 0, srefs) + throw(MissingException("cannot copy array with missing values to an array with element type $T")) end - for i in 1:n - dst[i] = src[i] + newlevels, ordered = mergelevels(isordered(dest), levels(dest), slevs) + # Orderedness cannot be preserved if the source was unordered and new levels + # need to be added: new comparisons would only be based on the source's order + # (this is consistent with what happens when adding a new level via setindex!) + ordered &= isordered(src) | (length(newlevels) == length(levels(dest))) + if ordered != isordered(dest) + isa(dest, SubArray) && throw(ArgumentError("cannot set ordered=$ordered on dest SubArray as it would affect the parent. Found when trying to set levels to $newlevels.")) + ordered!(dest, ordered) + end + + # Simple case: replace all values + if !isa(dest, SubArray) && dstart == dstart == 1 && n == length(dest) == length(src) + # Set index to reflect refs + levels!(dpool, T[]) # Needed in case src and dest share some levels + levels!(dpool, index(spool)) + + # Set final levels in their visible order + levels!(dpool, newlevels) + + copyto!(drefs, srefs) + else # More work to do: preserve some values (and therefore index) + levels!(dpool, newlevels) + + indexmap = indexin(index(spool), index(dpool)) + + @inbounds for i = 0:(n-1) + x = srefs[sstart+i] + drefs[dstart+i] = x > 0 ? indexmap[x] : 0 + end + end dest end -copyto!(dest::CatArrOrSub, src::AbstractArray) = +copyto!(dest::CatArrOrSub, src::CatArrOrSub) = copyto!(dest, 1, src, 1, length(src)) -copyto!(dest::CatArrOrSub, dstart::Integer, src::AbstractArray) = +copyto!(dest::CatArrOrSub, dstart::Integer, src::CatArrOrSub) = copyto!(dest, dstart, src, 1, length(src)) @static if VERSION >= v"0.7.0-DEV.3208" @@ -668,28 +694,15 @@ function Base.push!(A::CategoricalVector, item) A end -function Base.append!(dest::CategoricalVector, src::AbstractArray) - if isordered(dest) - slevelsdest = Set(levels(dest)) - if eltype(dest) >: Missing - push!(slevesdest, missing) - end - if !all(x -> x in slevesdest, src) - throw(ArgumentError("src contains values not allowed in dest which is ordered")) - end - else - if eltype(src) >: Missing && !(eltype(dest) >: Missing) && any(ismissing, src) - throw(MissingException("cannot append array with missing values to an array with element type $T")) - end - end - - len = length(dest) - len2 = length(src) - resize!(dest.refs, len + len2) +function Base.append!(A::CategoricalVector, B::CatArrOrSub) + levels!(A, union(levels(A), levels(B))) + len = length(A) + len2 = length(B) + resize!(A.refs, len + len2) for i = 1:len2 - dest[len + i] = src[i] + A[len + i] = B[i] end - return dest + return A end Base.empty!(A::CategoricalArray) = (empty!(A.refs); return A) From e6a51fadf23910aa2aa50fce6a3f8815dbd0a340 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bogumi=C5=82=20Kami=C5=84ski?= Date: Mon, 26 Nov 2018 11:21:26 +0100 Subject: [PATCH 3/9] improve copyto!, copy!, and append! --- src/array.jl | 107 +++++++++++++++++++++-------------------- test/13_arraycommon.jl | 27 +++++++++++ 2 files changed, 83 insertions(+), 51 deletions(-) diff --git a/src/array.jl b/src/array.jl index d6c3e5e5..84254aa2 100644 --- a/src/array.jl +++ b/src/array.jl @@ -405,57 +405,64 @@ copy(A::CategoricalArray) = deepcopy(A) CatArrOrSub{T, N} = Union{CategoricalArray{T, N}, SubArray{<:Any, N, <:CategoricalArray{T}}} where {T, N} -function copyto!(dest::CatArrOrSub{T, N}, dstart::Integer, - src::CatArrOrSub{<:Any, N}, sstart::Integer, - n::Integer) where {T, N} +function copyto!(dest::CatArrOrSub, dstart::Integer, + src::CatArrOrSub, sstart::Integer, + n::Integer) n == 0 && return dest - n < 0 && throw(ArgumentError(string("tried to copy n=", n, " elements, but n should be nonnegative"))) + n < 0 && throw(ArgumentError(string("tried to copy n=", n, + " elements, but n should be nonnegative"))) destinds, srcinds = LinearIndices(dest), LinearIndices(src) (dstart ∈ destinds && dstart+n-1 ∈ destinds) || throw(BoundsError(dest, dstart:dstart+n-1)) (sstart ∈ srcinds && sstart+n-1 ∈ srcinds) || throw(BoundsError(src, sstart:sstart+n-1)) - drefs = refs(dest) - srefs = refs(src) - dpool = pool(dest) - spool = pool(src) - - # try converting src to dest type to avoid partial copy corruption of dest - # in the event that the src cannot be copied into dest - slevs = convert(Vector{T}, levels(src)) - if eltype(src) >: Missing && !(eltype(dest) >: Missing) && !all(x -> x > 0, srefs) - throw(MissingException("cannot copy array with missing values to an array with element type $T")) - end - - newlevels, ordered = mergelevels(isordered(dest), levels(dest), slevs) - # Orderedness cannot be preserved if the source was unordered and new levels - # need to be added: new comparisons would only be based on the source's order - # (this is consistent with what happens when adding a new level via setindex!) - ordered &= isordered(src) | (length(newlevels) == length(levels(dest))) - if ordered != isordered(dest) - isa(dest, SubArray) && throw(ArgumentError("cannot set ordered=$ordered on dest SubArray as it would affect the parent. Found when trying to set levels to $newlevels.")) - ordered!(dest, ordered) - end - - # Simple case: replace all values - if !isa(dest, SubArray) && dstart == dstart == 1 && n == length(dest) == length(src) - # Set index to reflect refs - levels!(dpool, T[]) # Needed in case src and dest share some levels - levels!(dpool, index(spool)) - - # Set final levels in their visible order - levels!(dpool, newlevels) - - copyto!(drefs, srefs) - else # More work to do: preserve some values (and therefore index) - levels!(dpool, newlevels) - - indexmap = indexin(index(spool), index(dpool)) - - @inbounds for i = 0:(n-1) - x = srefs[sstart+i] - drefs[dstart+i] = x > 0 ? indexmap[x] : 0 + drefs = CategoricalArrays.refs(dest) + srefs = CategoricalArrays.refs(src) + dpool = CategoricalArrays.pool(dest) + spool = CategoricalArrays.pool(src) + + if isordered(dest) + remap = Vector{eltype(drefs)}(undef, length(levels(src))) + seen = falses(length(levels(src))) # needed as we have to dynamically collect levels + @inbounds for i in 0:n-1 + s = srefs[sstart+i] + if s == 0 + if eltype(dest) >: Missing + drefs[dstart+i] = 0 + else + throw(MissingException("cannot copy array with missing values to an array with element type $T")) + end + else + if !seen[s] + drefs[dstart+i] = remap[s] + else + seen[i] = true + rm = dpool.invindex[s] + remap[s] = rm + drefs[dstart+i] = rm + end + end + end + else + remap = Vector{eltype(drefs)}(undef, length(levels(src))) + seen = falses(length(levels(src))) # needed as we have to dynamically collect levels + @inbounds for i in 0:n-1 + s = srefs[sstart+i] + if s == 0 + if eltype(dest) >: Missing + drefs[dstart+i] = 0 + else + throw(MissingException("cannot copy array with missing values to an array with element type $T")) + end + else + if !seen[s] + drefs[dstart+i] = remap[s] + else + seen[i] = true + dest[dstart+i] = src[dstart+i] + remap[s] = dpool.invindex[drefs[dstart+i]] + end + end end - end dest @@ -469,7 +476,7 @@ copyto!(dest::CatArrOrSub, dstart::Integer, src::CatArrOrSub) = @static if VERSION >= v"0.7.0-DEV.3208" using Future - Future.copy!(dest::CatArrOrSub, src::CatArrOrSub) = + Future.copy!(dest::CatArrOrSub, src::AbstractArray) = copyto!(dest, 1, src, 1, length(src)) end @@ -694,14 +701,12 @@ function Base.push!(A::CategoricalVector, item) A end -function Base.append!(A::CategoricalVector, B::CatArrOrSub) - levels!(A, union(levels(A), levels(B))) +function Base.append!(A::CategoricalVector, B::AbstractArary) len = length(A) len2 = length(B) resize!(A.refs, len + len2) - for i = 1:len2 - A[len + i] = B[i] - end + # As in Base, A will be left modified if it is not possible to copy B to A + copyto!(A, len + 1, B) return A end diff --git a/test/13_arraycommon.jl b/test/13_arraycommon.jl index 2f0422f9..910f41fd 100644 --- a/test/13_arraycommon.jl +++ b/test/13_arraycommon.jl @@ -442,6 +442,33 @@ end end end + @testset "copyto! and copy! corner cases" begin + x = categorical([1:255;1:255], true) + y = categorical([1, 1000], true) + @test copyto!(x, 1, y, 1, 1)[1] == y[1] + @test_throws ErrorException copyto!(x, 1, y, 1, 1)[2] + + x = categorical([1,2,3]) + y = categorical([5,6,7]) + copyto!(x,y) + @test levels(x) = [1,2,3,5,6,7] + + @test copy!(x, [1,1,1]) == [1,1,1] + + x = categorical([1,2,3]) + y = categorical([5,6,7]) + @test copyto!(x, 1, y, 1, 1) == [5, 2, 3] + @test levels(x) == [1, 2, 3, 5] + + x = categorical([1,2,3], ordered=true) + y = categorical([10]) + @test_throws KeyError copyto!(x, 1, y, 1) + + x = categorical([1, 2, 3]) + using Future + @test Future.copy!(x, [1, 1, 1]) == [1, 1, 1] + end + @testset "resize!()" begin x = CategoricalArray{Union{T, String}}(["Old", "Young", "Middle", "Young"]) @test resize!(x, 3) === x From ff08d3519c01187dce9b4b39fcdb204dfae9f1af Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bogumi=C5=82=20Kami=C5=84ski?= Date: Mon, 26 Nov 2018 11:32:36 +0100 Subject: [PATCH 4/9] cleanup code --- src/array.jl | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/src/array.jl b/src/array.jl index 84254aa2..8e9af6dc 100644 --- a/src/array.jl +++ b/src/array.jl @@ -415,10 +415,10 @@ function copyto!(dest::CatArrOrSub, dstart::Integer, (dstart ∈ destinds && dstart+n-1 ∈ destinds) || throw(BoundsError(dest, dstart:dstart+n-1)) (sstart ∈ srcinds && sstart+n-1 ∈ srcinds) || throw(BoundsError(src, sstart:sstart+n-1)) - drefs = CategoricalArrays.refs(dest) - srefs = CategoricalArrays.refs(src) - dpool = CategoricalArrays.pool(dest) - spool = CategoricalArrays.pool(src) + drefs = refs(dest) + srefs = refs(src) + dpool = pool(dest) + spool = pool(src) if isordered(dest) remap = Vector{eltype(drefs)}(undef, length(levels(src))) @@ -432,7 +432,7 @@ function copyto!(dest::CatArrOrSub, dstart::Integer, throw(MissingException("cannot copy array with missing values to an array with element type $T")) end else - if !seen[s] + if seen[s] drefs[dstart+i] = remap[s] else seen[i] = true @@ -454,7 +454,7 @@ function copyto!(dest::CatArrOrSub, dstart::Integer, throw(MissingException("cannot copy array with missing values to an array with element type $T")) end else - if !seen[s] + if seen[s] drefs[dstart+i] = remap[s] else seen[i] = true @@ -701,7 +701,7 @@ function Base.push!(A::CategoricalVector, item) A end -function Base.append!(A::CategoricalVector, B::AbstractArary) +function Base.append!(A::CategoricalVector, B::AbstractArray) len = length(A) len2 = length(B) resize!(A.refs, len + len2) From c0f3f6164f0e47f2a204fb796f39a435a888e997 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bogumi=C5=82=20Kami=C5=84ski?= Date: Mon, 26 Nov 2018 12:07:33 +0100 Subject: [PATCH 5/9] fix reference --- src/array.jl | 2 +- test/13_arraycommon.jl | 11 +++++++++-- 2 files changed, 10 insertions(+), 3 deletions(-) diff --git a/src/array.jl b/src/array.jl index 8e9af6dc..7478f6d0 100644 --- a/src/array.jl +++ b/src/array.jl @@ -459,7 +459,7 @@ function copyto!(dest::CatArrOrSub, dstart::Integer, else seen[i] = true dest[dstart+i] = src[dstart+i] - remap[s] = dpool.invindex[drefs[dstart+i]] + remap[s] = drefs[dstart+i] end end end diff --git a/test/13_arraycommon.jl b/test/13_arraycommon.jl index 910f41fd..06c105cc 100644 --- a/test/13_arraycommon.jl +++ b/test/13_arraycommon.jl @@ -188,6 +188,14 @@ end levels!(x, ["Young", "Middle", "Old"]) ordered!(x, true) + for copyf! in (copy!, copyto!) + y = CategoricalArray{Union{T, String}}(["X", "Z", "Y", "X"]) + @test_throws KeyError copyf!(x, y) + end + + x = CategoricalArray{Union{T, String}}(["Old", "Young", "Middle", "Young"]) + levels!(x, ["Young", "Middle", "Old"]) + for copyf! in (copy!, copyto!) y = CategoricalArray{Union{T, String}}(["X", "Z", "Y", "X"]) @test copyf!(x, y) === x @@ -465,8 +473,7 @@ end @test_throws KeyError copyto!(x, 1, y, 1) x = categorical([1, 2, 3]) - using Future - @test Future.copy!(x, [1, 1, 1]) == [1, 1, 1] + @test copy!(x, [1, 1, 1]) == [1, 1, 1] end @testset "resize!()" begin From b4e8089ddacbd7015c06669bcd41c18b594d0151 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bogumi=C5=82=20Kami=C5=84ski?= Date: Mon, 26 Nov 2018 12:14:56 +0100 Subject: [PATCH 6/9] improve performance for non-missings --- src/array.jl | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/array.jl b/src/array.jl index 7478f6d0..d0fe51dd 100644 --- a/src/array.jl +++ b/src/array.jl @@ -425,7 +425,7 @@ function copyto!(dest::CatArrOrSub, dstart::Integer, seen = falses(length(levels(src))) # needed as we have to dynamically collect levels @inbounds for i in 0:n-1 s = srefs[sstart+i] - if s == 0 + if eltype(src) >: Missing && s == 0 if eltype(dest) >: Missing drefs[dstart+i] = 0 else @@ -435,7 +435,7 @@ function copyto!(dest::CatArrOrSub, dstart::Integer, if seen[s] drefs[dstart+i] = remap[s] else - seen[i] = true + seen[s] = true rm = dpool.invindex[s] remap[s] = rm drefs[dstart+i] = rm @@ -447,7 +447,7 @@ function copyto!(dest::CatArrOrSub, dstart::Integer, seen = falses(length(levels(src))) # needed as we have to dynamically collect levels @inbounds for i in 0:n-1 s = srefs[sstart+i] - if s == 0 + if eltype(src) >: Missing && s == 0 if eltype(dest) >: Missing drefs[dstart+i] = 0 else @@ -457,7 +457,7 @@ function copyto!(dest::CatArrOrSub, dstart::Integer, if seen[s] drefs[dstart+i] = remap[s] else - seen[i] = true + seen[s] = true dest[dstart+i] = src[dstart+i] remap[s] = drefs[dstart+i] end From 35d103f4076dc5e1792f3c7fdd93a1fa80d1c934 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bogumi=C5=82=20Kami=C5=84ski?= Date: Mon, 26 Nov 2018 13:26:39 +0100 Subject: [PATCH 7/9] fix typo and remove ambiguity --- src/array.jl | 7 +++---- test/13_arraycommon.jl | 13 ++++++++++++- 2 files changed, 15 insertions(+), 5 deletions(-) diff --git a/src/array.jl b/src/array.jl index d0fe51dd..2d189427 100644 --- a/src/array.jl +++ b/src/array.jl @@ -458,7 +458,7 @@ function copyto!(dest::CatArrOrSub, dstart::Integer, drefs[dstart+i] = remap[s] else seen[s] = true - dest[dstart+i] = src[dstart+i] + dest[dstart+i] = src[sstart+i] remap[s] = drefs[dstart+i] end end @@ -471,13 +471,12 @@ end copyto!(dest::CatArrOrSub, src::CatArrOrSub) = copyto!(dest, 1, src, 1, length(src)) -copyto!(dest::CatArrOrSub, dstart::Integer, src::CatArrOrSub) = - copyto!(dest, dstart, src, 1, length(src)) - @static if VERSION >= v"0.7.0-DEV.3208" using Future Future.copy!(dest::CatArrOrSub, src::AbstractArray) = copyto!(dest, 1, src, 1, length(src)) + Future.copy!(dest::CatArrOrSub, src::CatArrOrSub) = + copyto!(dest, 1, src, 1, length(src)) end similar(A::CategoricalArray{S, M, R}, ::Type{T}, diff --git a/test/13_arraycommon.jl b/test/13_arraycommon.jl index 06c105cc..94d4e542 100644 --- a/test/13_arraycommon.jl +++ b/test/13_arraycommon.jl @@ -200,7 +200,8 @@ end y = CategoricalArray{Union{T, String}}(["X", "Z", "Y", "X"]) @test copyf!(x, y) === x @test x == y - @test levels(x) == ["Young", "Middle", "Old", "X", "Y", "Z"] + # the additional levels are added in order of appereance in y + @test levels(x) == ["Young", "Middle", "Old", "X", "Z", "Y"] @test !isordered(x) end @@ -208,6 +209,16 @@ end levels!(x, ["Young", "Middle", "Old"]) ordered!(x, true) y = CategoricalArray{Union{T, String}}(["X", "Z", "Y", "X"]) + # Test with missing values + if T === Missing + x[3] = missing + y[3] = missing + end + @test_throws KeyError copyto!(x, 1, y, 2) + + x = CategoricalArray{Union{T, String}}(["Old", "Young", "Middle", "Young"]) + levels!(x, ["Young", "Middle", "Old"]) + y = CategoricalArray{Union{T, String}}(["X", "Z", "Y", "X"]) a = (Union{String, Missing})["Z", "Y", "X", "Young"] # Test with missing values if T === Missing From 5c5e571ce47c3a60118f9d0ae9450c898e59dd6b Mon Sep 17 00:00:00 2001 From: Milan Bouchet-Valat Date: Mon, 26 Nov 2018 14:43:45 +0100 Subject: [PATCH 8/9] Update src/array.jl Co-Authored-By: bkamins --- src/array.jl | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/array.jl b/src/array.jl index 2d189427..05b4a311 100644 --- a/src/array.jl +++ b/src/array.jl @@ -421,7 +421,7 @@ function copyto!(dest::CatArrOrSub, dstart::Integer, spool = pool(src) if isordered(dest) - remap = Vector{eltype(drefs)}(undef, length(levels(src))) + remap = similar(levels(src), eltype(drefs)) seen = falses(length(levels(src))) # needed as we have to dynamically collect levels @inbounds for i in 0:n-1 s = srefs[sstart+i] From 89ed2e4a95eb3338fb09a655edc5a4f1011b0080 Mon Sep 17 00:00:00 2001 From: Milan Bouchet-Valat Date: Mon, 26 Nov 2018 14:50:33 +0100 Subject: [PATCH 9/9] Update src/array.jl Co-Authored-By: bkamins --- src/array.jl | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/array.jl b/src/array.jl index 05b4a311..000060d4 100644 --- a/src/array.jl +++ b/src/array.jl @@ -436,7 +436,7 @@ function copyto!(dest::CatArrOrSub, dstart::Integer, drefs[dstart+i] = remap[s] else seen[s] = true - rm = dpool.invindex[s] + rm = index(dpool)[s] remap[s] = rm drefs[dstart+i] = rm end