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

Add special methods for Cartesian indexing of CartesianIndices for certain permissible indices #40344

Merged
merged 2 commits into from
Jul 19, 2021

Conversation

jishnub
Copy link
Contributor

@jishnub jishnub commented Apr 4, 2021

Closes #40314

Now a CartesianIndices, when indexed with ranges, Colon and compatible CartesianIndices returns another CartesianIndices instead of an Array of CartesianIndexes. The behavior is replicated while constructing views. This might have performance benefits.

julia> a = ones(2, 2)
2×2 Matrix{Float64}:
 1.0  1.0
 1.0  1.0

julia> c = CartesianIndices(a)
2×2 CartesianIndices{2, Tuple{Base.OneTo{Int64}, Base.OneTo{Int64}}}:
 CartesianIndex(1, 1)  CartesianIndex(1, 2)
 CartesianIndex(2, 1)  CartesianIndex(2, 2)

julia> c[1:2, :]
2×2 CartesianIndices{2, Tuple{UnitRange{Int64}, Base.OneTo{Int64}}}:
 CartesianIndex(1, 1)  CartesianIndex(1, 2)
 CartesianIndex(2, 1)  CartesianIndex(2, 2)

julia> c[c]
2×2 CartesianIndices{2, Tuple{Base.OneTo{Int64}, Base.OneTo{Int64}}}:
 CartesianIndex(1, 1)  CartesianIndex(1, 2)
 CartesianIndex(2, 1)  CartesianIndex(2, 2)

julia> c[c] === c
true

julia> c[:, :] === c
true

julia> @view c[:, :]
2×2 CartesianIndices{2, Tuple{Base.OneTo{Int64}, Base.OneTo{Int64}}}:
 CartesianIndex(1, 1)  CartesianIndex(1, 2)
 CartesianIndex(2, 1)  CartesianIndex(2, 2)

julia> (@view c[:, :]) === c
true

This doesn't work for combinations of range/colon and CartesianIndices though, as it dispatches on the ndims.

@jishnub jishnub closed this Apr 6, 2021
@jishnub jishnub reopened this Apr 6, 2021
Copy link
Sponsor Member

@johnnychen94 johnnychen94 left a comment

Choose a reason for hiding this comment

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

A clean implementation with carefully written tests. Most importantly, this sets up a solid ground for tile operations performance optimization that can be found in image processing and related fields. (Previously, view is the workaround for it).

R = CartesianIndices((2, 3, 4));
indices = (1:2, 1:2, 1:2);
@btime getindex($R, $indices...);
# master  58.120 ns (1 allocation: 272 bytes)
# PR 8.454 ns (0 allocations: 0 bytes)

@johnnychen94 johnnychen94 added domain:arrays [a, r, r, a, y, s] performance Must go faster labels Apr 6, 2021
@jishnub
Copy link
Contributor Author

jishnub commented Apr 6, 2021

Test failure seems unrelated

test/cartesian.jl Outdated Show resolved Hide resolved
@jishnub jishnub requested a review from stevengj May 22, 2021 13:10
@jishnub
Copy link
Contributor Author

jishnub commented May 27, 2021

gentle bump

@stevengj
Copy link
Member

Yes, the error in testset errorshow has cropped up a few times now (e.g. #30233, @ararslan), and seems unrelated.

@jishnub jishnub requested a review from johnnychen94 May 28, 2021 16:05
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] performance Must go faster
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Should views of CartesianIndices produce CartesianIndices?
4 participants