Skip to content
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

Fix inconsistant logical index behavior #45869

Merged
merged 7 commits into from
Dec 25, 2023
Merged

Conversation

N5N3
Copy link
Member

@N5N3 N5N3 commented Jun 30, 2022

  1. If we use BitArray/Array{Bool} to index, to_indices has an optimiztion for linear-iteratable case.
    But the corresponding check is not correct. IIUC, this optimization is legal only when the Boolen array is the only index provided.
    The first commit fix it and widen this optimization to all Boolen array.
    Before this PR
julia> A = rand(2,3,4); I = rand(Bool,3,4);

julia> A[1,I] == A[1,view(I,:,:)]
ERROR: BoundsError: attempt to access 2×3×4 Array{Float64, 3} at index [1, 3×4 Matrix{Bool}]

After

julia> A = rand(2,3,4); I = rand(Bool,3,4);

julia> A[1,I] == A[1,view(I,:,:)]
true
  1. On master, if the index/array has singleton trailing dimension, boundcheck of logical index show different behavior depending on the number of indexes provided.
    If there's only one index variable, singleton dimension wil not be ignored. The second commit fix it. (close Incorrect behavior of logical indexing #45867)

@N5N3 N5N3 added domain:arrays [a, r, r, a, y, s] kind:bugfix This change fixes an existing bug labels Jun 30, 2022
@N5N3 N5N3 force-pushed the logicalindex branch 2 times, most recently from a5a6184 to b73a6fa Compare June 30, 2022 09:22
@N5N3 N5N3 changed the title Fix the invalid optimization on trailing logical index. Fix inconsistant logical index behavior Jun 30, 2022
@N5N3 N5N3 marked this pull request as draft June 30, 2022 11:24
@N5N3 N5N3 force-pushed the logicalindex branch 6 times, most recently from 16220c3 to 10b912e Compare July 1, 2022 09:49
@N5N3 N5N3 marked this pull request as ready for review July 1, 2022 09:53
@ViralBShah
Copy link
Member

Who could review this? @StefanKarpinski?

@N5N3 N5N3 force-pushed the logicalindex branch 2 times, most recently from b252b79 to feb5aa1 Compare July 26, 2022 10:23
@StefanKarpinski
Copy link
Sponsor Member

@mbauman would be a good person to review when he has the chance.

@N5N3 N5N3 force-pushed the logicalindex branch 2 times, most recently from 5d6a1ed to 1e99dcc Compare April 23, 2023 14:25
Copy link
Sponsor Member

@mbauman mbauman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry to leave this languishing so long — I'm pretty sure I've tried reviewing it long ago but it's pretty complicated... and then I forgot about it. See my comments, I think it'd be good to simplify/coalesce some of this logic while we're here instead of making it yet more complicated. It feels like this is missing a primitive — maybe an internal function like _peel_axes_for_index (that would effectively do a peel for the appropriate number of axes) would help simplify many of these functions? I've been trying to get such a refactor written but I've simply not had time to see it to the end yet.

base/multidimensional.jl Outdated Show resolved Hide resolved
base/multidimensional.jl Outdated Show resolved Hide resolved
base/abstractarray.jl Outdated Show resolved Hide resolved
This no more influence inference on master.
…only index variable.

`zeros(2,3,4)[2, 1]` is always invalid, thus this optimization is illegal for trailing dimension.
Implement `checkbounds` using `to_indices` style.
The behavior of non-logical doesn't change.
While for `AbstractArray{Bool,N}`s, they would behaves like `Vector{CartesianIndex{N}}` after this commit, if its shape matches the indexed dimensions.
Thus the trailing singleton dimensions in boolean array would be ingnored only when we have comsumed all of the src dimensions.

The test change includes:
1. disallow `zeros(2,2)[trues(1,2), 1]`
2. allow `zeros(2,2)[trues(2,2,1)]`
base/multidimensional.jl Outdated Show resolved Hide resolved
base/abstractarray.jl Outdated Show resolved Hide resolved
N5N3 and others added 2 commits December 21, 2023 08:34
@N5N3 N5N3 merged commit 933a83a into JuliaLang:master Dec 25, 2023
4 of 7 checks passed
@N5N3 N5N3 deleted the logicalindex branch December 25, 2023 12:54
@maleadt
Copy link
Member

maleadt commented Jan 17, 2024

In order to adapt CUDA.jl to this, which requires iteratable logical indices too, is this the correct approach?

# we cannot use Base.LogicalIndex, which does not support indexing but requires iteration.
Base.to_index(::CuArray, I::AbstractArray{Bool}) = findall(I)
if VERSION >= v"1.11.0-DEV.1157"
    # for this PR
    Base.to_indices(A::CuArray, I::Tuple{AbstractArray{Bool}}) = (Base.to_index(A, I[1]),)
else
    # what we used to have
    Base.to_indices(A::CuArray, inds,
                    I::Tuple{Union{Array{Bool,N}, BitArray{N}}}) where {N} =
        (Base.to_index(A, I[1]),)
end

@N5N3
Copy link
Member Author

N5N3 commented Jan 17, 2024

is this the correct approach?

Yes, it should recover the needed overloading.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
domain:arrays [a, r, r, a, y, s] kind:bugfix This change fixes an existing bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Incorrect behavior of logical indexing
5 participants