Make sure that new CategoricalArray allocates new pool and refs #110
Conversation
|
@nalimilan similarly here - errors are unrelated (due to changes in 0.7) |
src/array.jl
Outdated
| @@ -154,14 +154,23 @@ CategoricalMatrix{T}(m::Int, n::Int; ordered=false) where {T} = | |||
|
|
|||
| ## Constructors from arrays | |||
|
|
|||
| # This method is needed to ensure that a copy of the pool is always made | |||
| # This method is needed to ensure that a copy of the refs and pool is always made | |||
| # so that ordered!() does not affect the original array | |||
There was a problem hiding this comment.
Just remove this line, now we make a copy because that's what the function is documented to do.
src/array.jl
Outdated
| needs_copy = true | ||
| end | ||
| if needs_copy | ||
| res = CategoricalArray{V, N}(refs, pool) |
There was a problem hiding this comment.
Better always call that constructor, anyway CategoricalArray is an immutable and it should be cheap.
test/11_array.jl
Outdated
| @@ -638,4 +638,14 @@ end | |||
| @test unique(x) == collect(1500:-1:1) | |||
| end | |||
|
|
|||
| @testset "categorical() makes a copy of pool and refs" begin | |||
There was a problem hiding this comment.
Put this in arraycommon.jl instead. It would make sense to test with an array supporting missing too, and with the CategoricalArray{T, R} form with equal and different types, just in case.
test/11_array.jl
Outdated
| @testset "categorical() makes a copy of pool and refs" begin | ||
| x = categorical(1:10) | ||
| y = categorical(x) | ||
| @test !(x.refs === y.refs) |
src/array.jl
Outdated
| function CategoricalArray{T, N, R}(A::CategoricalArray{S, N, Q}; | ||
| ordered=_isordered(A)) where {S, T, N, Q, R} | ||
| V = unwrap_catvaluetype(T) | ||
| res = convert(CategoricalArray{V, N, R}, A) | ||
| if res.pool === A.pool # convert() only makes a copy when necessary | ||
| res = CategoricalArray{V, N}(res.refs, deepcopy(res.pool)) | ||
| needs_copy = false |
There was a problem hiding this comment.
needs_copy is no longer needed.
Codecov Report
@@ Coverage Diff @@
## master #110 +/- ##
==========================================
+ Coverage 97.78% 97.81% +0.02%
==========================================
Files 9 9
Lines 678 685 +7
==========================================
+ Hits 663 670 +7
Misses 15 15
Continue to review full report at Codecov.
|
Following Issue #107.
I test for new
refsand newpoolseparately as I guess it is safest.