Skip to content

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

Merged
merged 7 commits into from
May 19, 2025
Merged

fix and test mixed CartesianIndex #260

merged 7 commits into from
May 19, 2025

Conversation

rafaqz
Copy link
Collaborator

@rafaqz rafaqz commented Apr 21, 2025

Fixes #259

@lupemba hopefully this solves your issue

@lupemba
Copy link

lupemba commented Apr 26, 2025

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.

@rafaqz
Copy link
Collaborator Author

rafaqz commented Apr 26, 2025

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.

@lupemba
Copy link

lupemba commented Apr 26, 2025

Yes in the latest commit on this branch. The error comes from "DiskArrays.jl/src/subarray.jl:65"

i2 = _replace_colon.(size(a), i) size(a) has length 3 and i only has length 2 so broadcasting fails.

ERROR: DimensionMismatch: arrays could not be broadcast to a common size: a has axes Base.OneTo(3) and b has axes Base.OneTo(2)
Stacktrace:
  [1] _bcs1
    @ ./broadcast.jl:528 [inlined]
  [2] _bcs
    @ ./broadcast.jl:522 [inlined]
  [3] broadcast_shape(::Tuple{Base.OneTo{Int64}}, ::Tuple{Base.OneTo{Int64}})
    @ Base.Broadcast ./broadcast.jl:516
  [4] combine_axes
    @ ./broadcast.jl:497 [inlined]
  [5] _axes
    @ ./broadcast.jl:236 [inlined]
  [6] axes
    @ ./broadcast.jl:234 [inlined]
  [7] copy
    @ ./broadcast.jl:1099 [inlined]
  [8] materialize
    @ ./broadcast.jl:872 [inlined]
  [9] view(::AccessCountDiskArray{…}, ::CartesianIndex{…}, ::UnitRange{…})
    @ DiskArrays ~/Documents/git repos/DiskArrays.jl/src/subarray.jl:65
 [10] top-level scope
    @ ~/Documents/git repos/DiskArrays.jl/small_test.jl:6
Some type information was truncated. Use `show(err)` to see complete types.

@rafaqz
Copy link
Collaborator Author

rafaqz commented Apr 26, 2025

Ok thanks, I'll fix that tomorrow or monday

@rafaqz
Copy link
Collaborator Author

rafaqz commented May 5, 2025

Ok so it turns out this fails with any array at all! Its just julias end doesn't work with CartesianIndex in the mix, it will lower to lastindex(a, 2) rather than lastindex(a, 3).

@view a[CartesianIndex(1, 2), 1:end] # fails

@rafaqz
Copy link
Collaborator Author

rafaqz commented May 5, 2025

@meggart could you have a look at this? it would be good to merge for @lupemba s dependent PR

@lupemba
Copy link

lupemba commented May 6, 2025

Ok so it turns out this fails with any array at all! Its just julias end doesn't work with CartesianIndex in the mix, it will lower to lastindex(a, 2) rather than lastindex(a, 3).

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 

Copy link
Collaborator

@meggart meggart left a 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

@meggart
Copy link
Collaborator

meggart commented May 19, 2025

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 CartesianIndex than there were actual dimensions. I re-implemented the fix with a (probably unnecessary) additional argument check. Thanks for taking care.

@meggart meggart self-requested a review May 19, 2025 08:50
@meggart meggart merged commit b0a4a7b into main May 19, 2025
10 checks passed
@rafaqz rafaqz deleted the cartesian_index branch May 19, 2025 18:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Mixed indexing fail. a[CartesianIndex(1, 2), 1:end]
3 participants