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 constructors for CartesianIndices #142

Merged
merged 2 commits into from
Sep 13, 2020

Conversation

jishnub
Copy link
Member

@jishnub jishnub commented Sep 13, 2020

Fixes #71 . With this PR the following are possible:

julia> OffsetArray{Float64}(undef, CartesianIndex(3,4,-1):CartesianIndex(4,4,0))
2×1×2 OffsetArray(::Array{Float64,3}, 3:4, 4:4, -1:0) with eltype Float64 with indices 3:4×4:4×-1:0:
[:, :, -1] =
 6.93496739309444e-310
 6.93493894159696e-310

[:, :, 0] =
 6.9349593623032e-310
 6.93496425153495e-310

julia> v = rand(2,2,2);

julia> OffsetArray(v, :, CartesianIndex(2,2):CartesianIndex(3,3))
2×2×2 OffsetArray(::Array{Float64,3}, 1:2, 2:3, 2:3) with eltype Float64 with indices 1:2×2:3×2:3:
[:, :, 2] =
 0.679194  0.0652147
 0.684409  0.968799

[:, :, 3] =
 0.654027  0.599548
 0.665052  0.75195

julia> OffsetArray(v, :, CartesianIndex(2):CartesianIndex(3), 4:5)
2×2×2 OffsetArray(::Array{Float64,3}, 1:2, 2:3, 4:5) with eltype Float64 with indices 1:2×2:3×4:5:
[:, :, 4] =
 0.679194  0.0652147
 0.684409  0.968799

[:, :, 5] =
 0.654027  0.599548
 0.665052  0.75195

Not too sure if CartesianIndices(..).indices is future-proof.

@codecov
Copy link

codecov bot commented Sep 13, 2020

Codecov Report

Merging #142 into master will increase coverage by 0.89%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #142      +/-   ##
==========================================
+ Coverage   90.49%   91.39%   +0.89%     
==========================================
  Files           2        2              
  Lines         221      244      +23     
==========================================
+ Hits          200      223      +23     
  Misses         21       21              
Impacted Files Coverage Δ
src/OffsetArrays.jl 91.50% <100.00%> (+1.10%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d58dda0...3b1a70f. Read the comment docs.

@johnnychen94
Copy link
Member

OffsetArray(v, :, CartesianIndex(2,2):CartesianIndex(3,3))

This looks pretty handy at the first sight. However, I wouldn't expect fusions like this work and this seems like an overdesign to me. Do we have similar usage in Base or other Julia packages?

It seems that there're other constructors that haven't yet get benefit from this: zeros, ones, similar, etc. Does this PR plan to support that?

@jishnub
Copy link
Member Author

jishnub commented Sep 13, 2020

Slicing implements something like this, so perhaps this might serve to wrap the slice with the indices? Eg something like this:

julia> a = reshape(1:24, 3, 4, 2)
3×4×2 reshape(::UnitRange{Int64}, 3, 4, 2) with eltype Int64:
[:, :, 1] =
 1  4  7  10
 2  5  8  11
 3  6  9  12

[:, :, 2] =
 13  16  19  22
 14  17  20  23
 15  18  21  24

julia> inds = (:, CartesianIndex(2,1):CartesianIndex(3,2))
(Colon(), CartesianIndex{2}[CartesianIndex(2, 1) CartesianIndex(2, 2); CartesianIndex(3, 1) CartesianIndex(3, 2)])

julia> oa = OffsetArray(a[inds...], inds...)
3×2×2 OffsetArray(::Array{Int64,3}, 1:3, 2:3, 1:2) with eltype Int64 with indices 1:3×2:3×1:2:
[:, :, 1] =
 4  7
 5  8
 6  9

[:, :, 2] =
 16  19
 17  20
 18  21

julia> oa[1,2,1] == a[1,2,1]
true

I think it might be convenient to have constructors with any Base index type that might be used in slicing.

The reason I didn't implement zeros etc is type-piracy, better to wait for a resolution to #87 to see how to proceed.

Copy link
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.

This is nice work!

@jishnub I think this one is ready? Or do you have any other more commits in?

Comment on lines +95 to +99
function stripCartesianIndices(inds::Tuple{CartesianIndices{1},Vararg{Any}})
I = first(inds)
Ir = convert(Tuple{AbstractUnitRange{Int}}, I) |> first
(Ir, stripCartesianIndices(tail(inds))...)
end
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
function stripCartesianIndices(inds::Tuple{CartesianIndices{1},Vararg{Any}})
I = first(inds)
Ir = convert(Tuple{AbstractUnitRange{Int}}, I) |> first
(Ir, stripCartesianIndices(tail(inds))...)
end
stripCartesianIndices(inds::Tuple{CartesianIndices{1},Vararg{Any}}) = (first(inds).indices..., stripCartesianIndices(tail(inds))...)

Copy link
Member Author

Choose a reason for hiding this comment

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

I was trying to avoid using CartesianIndices(..).indices, as I'm not entirely sure that it's future-proof. The convert route was suggested by Tim Holy to get around this, and seems to produce the same lowered code

julia> @code_lowered convert(Tuple{AbstractUnitRange{Int}}, c)
CodeInfo(
1%1 = Base.getproperty(R, :indices)
└──      return %1
)

I can condense these a bit if that helps in legibility.

Copy link
Member

Choose a reason for hiding this comment

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

Oh sorry, I missed that part of the conversation. These two are actually equivalent.

Future-proof seems okay to me.

Comment on lines +112 to +115
function splitCartesianIndices(c::CartesianIndices)
c1, ct = Base.IteratorsMD.split(c, Val(1))
(c1, splitCartesianIndices(ct)...)
end
Copy link
Member

Choose a reason for hiding this comment

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

Good to know this split usage.

I tried the following, too. But this one is much slower than your version.

splitCartesianIndices(c::CartesianIndices) = IdOffsetRange.(c.indices)

IdOffsetRange could perhaps be optimized a bit. Of course not in this PR.

@jishnub
Copy link
Member Author

jishnub commented Sep 13, 2020

I don't think that there are more commits to this

@johnnychen94
Copy link
Member

Has @timholy given you an invitation to this repo? If so, please merge when you feel it's ready!

@jishnub jishnub merged commit 55af8ad into JuliaArrays:master Sep 13, 2020
@jishnub jishnub deleted the CartesianIndices branch September 21, 2020 05:51
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.

Allow for constructing arrays with CartesianRange
2 participants