-
Notifications
You must be signed in to change notification settings - Fork 21
fix and test mixed CartesianIndex #260
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
Conversation
I just did a small test and discovered that it doesn't work for the views. using DiskArrays: ReshapedDiskArray, PermutedDiskArray
using DiskArrays.TestTypes
a = AccessCountDiskArray(reshape(1:24, 2, 3, 4), chunksize=(2, 2, 2))
a[CartesianIndex(1, 2), 1:end] # works now
@view a[CartesianIndex(1, 2), 1:end] # fails I don't think I need the functionality but I just wanted to share it right away before I forget. |
In the latest push? I tried to fix view too, it works in the tests now. Have to check why this argument combo doesn't work. |
Yes in the latest commit on this branch. The error comes from "DiskArrays.jl/src/subarray.jl:65"
|
Ok thanks, I'll fix that tomorrow or monday |
Ok so it turns out this fails with any array at all! Its just julias @view a[CartesianIndex(1, 2), 1:end] # fails |
Interesting, I will add an issue to the Julia repository to document this. a = reshape(1:(2*3*4),(4,3,2))
a[CartesianIndex(1, 2), :] # works
a[1,2, 1:end] # works
a[CartesianIndex(1, 2), 1:end] # fails |
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.
Thanks a lot for digging into this. Looks good in general, except the implementation of process_index
Ok, I finally understood the bug. It was not view-related but could be induced by a getindex on a normal DiskArray as well when indexing with a longer |
Fixes #259
@lupemba hopefully this solves your issue