Implementing repeat for PooledArray #67
Conversation
This makes the repeat of a PooledArray more efficient.
src/PooledArrays.jl
Outdated
| @@ -601,4 +601,7 @@ _perm(o::F, z::V) where {F, V} = Base.Order.Perm{F, V}(o, z) | |||
|
|
|||
| Base.Order.Perm(o::Base.Order.ForwardOrdering, y::PooledArray) = _perm(o, fast_sortable(y)) | |||
|
|
|||
| Base.repeat(x::PooledArray, counts...) = PooledArray(RefArray(repeat(x.refs, counts...)), copy(x.invpool)) | |||
There was a problem hiding this comment.
copying of pool and invpool is not needed and should be avoided. Please check the implementation of other operations to see how copy-on-write is implemented. Thank you!
There was a problem hiding this comment.
also could you please add some tests?
There was a problem hiding this comment.
Thanks for comments. Updated the function and add some tests.
There was a problem hiding this comment.
Ah - now I see. Such ambiguities are super annoying (and that is why having a detailed test coverage is crucial 😄). Thank you!
Codecov Report
@@ Coverage Diff @@
## main #67 +/- ##
==========================================
+ Coverage 88.10% 88.36% +0.25%
==========================================
Files 1 1
Lines 269 275 +6
==========================================
+ Hits 237 243 +6
Misses 32 32
Continue to review full report at Codecov.
|
src/PooledArrays.jl
Outdated
| @@ -601,4 +601,14 @@ _perm(o::F, z::V) where {F, V} = Base.Order.Perm{F, V}(o, z) | |||
|
|
|||
| Base.Order.Perm(o::Base.Order.ForwardOrdering, y::PooledArray) = _perm(o, fast_sortable(y)) | |||
|
|
|||
| function Base.repeat(x::PooledArray{T, R, N, RA}, counts...) where {T, R<:Integer, N, RA} | |||
There was a problem hiding this comment.
I would assume that the {T, R, N, RA} part is not needed. Why have you added it?
There was a problem hiding this comment.
based on CI output, it seems it is needed for Julia 1.0.5?
There was a problem hiding this comment.
It seems the problem should be solved in another way. I updated it.
test/runtests.jl
Outdated
| pa1 = PooledArray(["one", "two"]) | ||
| # check it didn't mess the other pa | ||
| @test pa3 == [1, 2, 3, 1, 2, 3] | ||
|
|
There was a problem hiding this comment.
This doesn't check much AFAICT: no significant operation has been performed since the last check.
| pa1 = PooledArray(["one", "two"]) | |
| # check it didn't mess the other pa | |
| @test pa3 == [1, 2, 3, 1, 2, 3] | |
| pa1 = PooledArray(["one", "two"]) |
There was a problem hiding this comment.
Should I remove it?
There was a problem hiding this comment.
What's what the suggestion does?
There was a problem hiding this comment.
Sorry, I am lost. What is your suggestion on lines 478-480?
There was a problem hiding this comment.
Not sure why GitHub is confused about it, but I just suggested dropping lines 479-481.
There was a problem hiding this comment.
I see,
GitHub doesn't allow to commit suggestion (the suggestion is invalid...)!
Co-authored-by: Milan Bouchet-Valat <nalimilan@club.fr>
Co-authored-by: Milan Bouchet-Valat <nalimilan@club.fr>
|
Thanks! |
the
repeatfunction forPooledArraybefore
after