add dims kwarg #247
add dims kwarg #247
Conversation
src/mapreduce.jl
Outdated
| out_dims = [size(a)...] | ||
| out_dims[_dims] .= 1 | ||
| out = fill($init(eltype(a)), out_dims...) | ||
| for c in eachchunk(a) | ||
| out_c = [c...] | ||
| out_c[_dims] .= Ref(1:1) | ||
| out[out_c...] .= $acum.(out[out_c...], Base.$_fname(f, a[c...], dims)) |
There was a problem hiding this comment.
Comments for each line would help for understanding the logic.
Also splatting a vector isn't type stable, maybe better to convert back to tuple and pass it through a function barrier before the loop? We actually know the length too so you could use ntuple to make it.
src/mapreduce.jl
Outdated
| @eval function Base.$_fname(f::Function, a::AbstractDiskArray, dims) | ||
| _dims = typeof(dims)<:Tuple ? [dims...] : typeof(dims)<:Number ? [dims] : dims | ||
| out_dims = [size(a)...] | ||
| out_dims[_dims] .= 1 |
There was a problem hiding this comment.
out_dims = ntuple(Val{N}()) do i
i in _dims ? 1 : size(a)[i]
end Something like this should be the same but type stable for splats. N needs a where N on the array ndims type parameter.
(actually the one below is more important as its in the loop, but theyre kind of the same)
There was a problem hiding this comment.
added a commit with this code. the out array is still type unstable. not sure how to fix that given it's eltype is calculated.
|
also need to search for something other than "DiskGenerator" in the output of |
src/mapreduce.jl
Outdated
| out_dims = ntuple(Val(N)) do i | ||
| i in _dims ? 1 : size(a)[i] | ||
| end | ||
| out = fill($init(Base.return_types(f, (T,))[1]), out_dims...) |
There was a problem hiding this comment.
| out = fill($init(Base.return_types(f, (T,))[1]), out_dims...) | |
| T1 = Base.promote_op(f, T) | |
| out = fill($init(T1), out_dims...) |
I think this is more likely to be stable? (untested)
(also seems you have 8 space indenting turned on somewhere instead of 4)
|
Why exactly do we need these special implementations? Has something in the Julia mapreduce internals changed so that all of these don't hit the |
which PR? and no, nothing falls back to mapreduce now as the internals have changed. #246 |
|
Right yes we have this code already: https://github.com/JuliaIO/DiskArrays.jl/blob/main/src/mapreduce.jl#L13-L26 Probably we need to hook this up again? It could also do with some comments and cleanup so its clear what its all for |
|
oops, i take that back. |
not battle tested yet. and might as well add an
initkwarg too