-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Speed-up repeat for AbstractArrays #20635
Conversation
@nanosoldier |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great!
|
||
indices_in = Vector{Int}(ndims_in) | ||
indices_out = Vector{Int}(ndims_out) | ||
inner=ntuple(n->1, Val{ndims(A)}), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Whoever made ndims
inferable on 0.6, thanks!
base/abstractarraymath.jl
Outdated
n = inner[i] | ||
inner_indices[i] = (1:n) + ((c.I[i] - 1) * n) | ||
end | ||
R[inner_indices...] = A[c.I...] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
rhs can be just A[c]
(and it will be slightly faster because there's no splatting)
base/abstractarraymath.jl
Outdated
for c in CartesianRange(indices(A)) | ||
for i in 1:ndims(A) | ||
n = inner[i] | ||
inner_indices[i] = (1:n) + ((c.I[i] - 1) * n) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
c[i]
R[indices(A)...] = A | ||
else | ||
inner_indices = [1:n for n in inner] | ||
for c in CartesianRange(indices(A)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would this be any better as map((n,i)->(1:n)+(i-1)*n, inner, c.I)
? i.e., all tuples rather than using an array.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried that, the problem is that inner
and c.I
can be of different length.
Your benchmark job has completed - possible performance regressions were detected. A full report can be found here. cc @jrevels |
This case broke: julia> repeat([], outer=0)
ERROR: BoundsError: attempt to access 0-element UnitRange{Int64} at index [0]
Stacktrace:
[1] throw_boundserror(::UnitRange{Int64}, ::Int64) at ./abstractarray.jl:417
[2] getindex at ./range.jl:460 [inlined]
[3] _repeat at ./abstractarraymath.jl:423 [inlined]
[4] #repeat#110(::Tuple{Int64}, ::Int64, ::Function, ::Array{Any,1}) at ./abstractarraymath.jl:365
[5] (::Base.#kw##repeat)(::Array{Any,1}, ::Base.#repeat, ::Array{Any,1}) at ./<missing>:0
This triggers a failure in DataTables. |
thanks for this @pabloferz !!! |
This broke another use case of repeat as well: julia> repeat([[1,2], [3,4]]; inner=2)
ERROR: MethodError: Cannot `convert` an object of type Int64 to an object of type Array{Int64,1}
This may have arisen from a call to the constructor Array{Int64,1}(...),
since type constructors fall back to convert methods.
Stacktrace:
[1] setindex!(::Array{Array{Int64,1},1}, ::Array{Int64,1}, ::UnitRange{Int64}) at ./array.jl:573
[2] _repeat at ./abstractarraymath.jl:411 [inlined]
[3] #repeat#117(::Int64, ::Tuple{Int64}, ::Function, ::Array{Array{Int64,1},1}) at ./abstractarraymath.jl:366
[4] (::Base.#kw##repeat)(::Array{Any,1}, ::Base.#repeat, ::Array{Array{Int64,1},1}) at ./<missing>:0 The result should be julia> repeat([[1,2], [3,4]]; inner=2)
4-element Array{Array{Int64,1},1}:
[1,2]
[1,2]
[3,4]
[3,4] as it was before this commit. Note that the following work fine though: julia> repeat([[1,2],[3,4]]; outer=2)
4-element Array{Array{Int64,1},1}:
[1, 2]
[3, 4]
[1, 2]
[3, 4] julia> repeat([(1,2),(3,4)]; inner=2)
4-element Array{Tuple{Int64,Int64},1}:
(1, 2)
(1, 2)
(3, 4)
(3, 4) |
Thanks @braydenware ; could you file an issue? |
I'd say this should fix #15553. Compared to the OP in that issue I get times of ~3ms vs ~600ms on master.
Also, compared to this JuliaLang/LinearAlgebra.jl#402, I get
This
repeat
is still type-unstable, but that should improve when inference and keyword arguments improve.