Skip to content

Broadcast in Julia 1.11 requires getindex/setindex! to work on multiple integer indexing #56295

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

Closed
charleskawczynski opened this issue Oct 22, 2024 · 7 comments
Labels
broadcast Applying a function over a collection

Comments

@charleskawczynski
Copy link
Contributor

This didn't used to be the case in Julia 1.10, and this new requirement means that we cannot reserve this indexing for special behavior.

For example, the fix needed: CliMA/ClimaCore.jl#2018

@charleskawczynski
Copy link
Contributor Author

I'll try to make a reproducer at some point

@mbauman mbauman added the broadcast Applying a function over a collection label Oct 22, 2024
@mbauman
Copy link
Member

mbauman commented Oct 22, 2024

Interesting — these look like they're all non-AbstractArrays who have axes and previously supported get/setindex with CartesianIndex alone?

@charleskawczynski
Copy link
Contributor Author

they're all non-AbstractArrays who have axes and previously supported get/setindex with CartesianIndex alone?

Yeah, that's accurate. I put a fair bit of work into removing multiple integer indexing. We'd like to reserve linear indexing (i.e., multiple integer indexing) so that we can special-case fixes for #28126 (including the GPU-variant related issue resolution efforts: JuliaGPU/GPUArrays.jl#454, JuliaGPU/GPUArrays.jl#464).

@charleskawczynski
Copy link
Contributor Author

Is there anything more that I can do to help this get fixed? (or is there any way we can work around this?)

@mbauman
Copy link
Member

mbauman commented Dec 24, 2024

I mean, it’s not abundantly obvious to me that broadcast should exclusively use one or the other — or which one in the first place. I certainly couldn’t have told you that broadcast always generated CartesianIndex-based getindexes on 1.10.

At a caller level, I have always implicitly assumed that the two should be perfectly equivalent. At the implementation level, AbstractArray considers n-ary Int to be the “canonical“ one. Having different behaviors between the two seems fraught for many algorithms, not just broadcast.

@charleskawczynski
Copy link
Contributor Author

Ok, yes, I agree with you regarding design choices. We’re pretty heavily using broadcast in the CliMA org, and the 2x slowdown from Cartesian indexing is important enough that we need an alternative—whether that is some way to specialize on linear indexing, or roll our own custom version of broadcast.

I’ll look into the former, first, since that may be significantly less work.

@charleskawczynski
Copy link
Contributor Author

It turns out that I was actually able to fix this with dispatch at a higher level.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
broadcast Applying a function over a collection
Projects
None yet
Development

No branches or pull requests

2 participants