[JLArrays] allow mapreducedim! into wrapper - #754
Conversation
|
Hmm, this is annoying! Looks like all |
Discovered in JuliaGPU/GPUArrays.jl#754. Once this is approved, I will open PRs for all the other backends as well.
`mapreducedim!` reshapes the output container to match the number of dimensions of the input, but then returned that reshaped array instead of the one it was handed. Keep a reference to the original and return that. Same as JuliaGPU/CUDA.jl#3219, discovered in JuliaGPU/GPUArrays.jl#754.
`mapreducedim!` reshapes the output container to match the number of dimensions of the input, but then returned that reshaped array instead of the one it was handed. Keep a reference to the original and return that. Same as JuliaGPU/CUDA.jl#3219, discovered in JuliaGPU/GPUArrays.jl#754.
|
Can you fix this up into a JLArrays commit and a tests commit and then we won't squash |
|
Ok, I can do that! Looks like Metal tests are still failing though, do you have any ideas why? |
It's unrelated to this. You can work around it by using Good catch! JuliaGPU/Metal.jl#907 |
This comment was marked as resolved.
This comment was marked as resolved.
* don't return `ReshapedArray` from `mapreducedim!` Discovered in JuliaGPU/GPUArrays.jl#754. Once this is approved, I will open PRs for all the other backends as well. * change other return as well * fix doctest
2f1823e to
ca32291
Compare
|
I split it up into two nicely separated commits. Now just needs JuliaGPU/Metal.jl#908 to fix the
I didn't just want to work around it since I think it's good to exercise the |
|
Hey @simeonschaub. I'm planning on releasing 11.5.9 once #755 passes tests, but I think we can get this with the workaround in. Can you edit the JLArrays commit to bump JLArrays to 0.3.2, and the other one to implement the "workaround". And then open a new PR/issue to keep track of the Metal regression? |
Previously, e.g. `sum!(transpose(v), M)` would error
Test that `mapreducedim!` works as expected if `R` is a wrapper type like `transpose` or `adjoint`. Previously, all the GPU backends also returned a reshaped array here, which is inconsistent with base and could cause issues with the aforementioned wrapper types. Test that `R` is returned directly.
ca32291 to
0328acd
Compare
|
Sure, done!
I've added a reference to JuliaGPU/Metal.jl#907, I think we can just track this there |
* don't return `ReshapedArray` from `mapreducedim!` Discovered in JuliaGPU/GPUArrays.jl#754. Once this is approved, I will open PRs for all the other backends as well. * change other return as well * fix doctest
2 participants
Previously, e.g.
sum!(transpose(v), M)would error