RFC: use offset axes for offset arrays #27038

mbauman · 2018-05-09T00:04:22Z

This is a shot at solving some of the difficulties we faced in #26682 (comment) by ensuring the axes of an offset array are similarly offset such that ax[i] == i for each ax in axes(A). This is an idea that's been bouncing around for a while — I believe Andy Ferris first posited it. It does seem to solve some of the issues we faced, but I'm still not quite to the end of this tunnel.

The goal here is to fully support LinearIndices and CartesianIndices as generic replacements for ind2sub and sub2ind for both normal and offset arrays. The issue with the design in #26682 (which achieves this goal) is that it makes CartesianIndices((20:30, 20:30)) an offset array itself! This was a breaking change for folks that took full advantage of the CartesianIndices object. This PR, instead, is experimenting with making these indices-wrapper types reflect the axes of their indices and making OffsetArrays have offset axes. The goal is to enable the following properties for both normal and offset arrays A:

A == A[axes(A)...] == A[LinearIndices(A)] == A[CartesianIndices(A)]
LinearIndices(A) == LinearIndices(A)[axes(A)...] == LinearIndices(A)[LinearIndices(A)] == LinearIndices(A)[CartesianIndices(A)]
CartesianIndices(A) == CartesianIndices(A)[axes(A)...] == CartesianIndices(A)[LinearIndices(A)] == CartesianIndices(A)[CartesianIndices(A)]

One thing that could use some bikeshedding is the name and use of Base.Slice — do we export it? It's now pulling double-duty: OffsetArrays use it both as their axis type and as a flag to SubArray that a given index represents an entire dimension. It might be interesting to try experimenting with returning Slice{OneTo}s as the axes to normal arrays, too. For example, view(A, axes(A)...) isn't ever IndexLinear, even if view(A, :, :, …) would be.

mbauman · 2018-05-09T20:00:57Z

base/array.jl

@@ -545,7 +545,7 @@ else
 end

 _array_for(::Type{T}, itr, ::HasLength) where {T} = Vector{T}(undef, Int(length(itr)::Integer))
-_array_for(::Type{T}, itr, ::HasShape) where {T} = similar(Array{T}, axes(itr))::Array{T}
+_array_for(::Type{T}, itr, ::HasShape) where {T} = similar(Array{T}, axes(itr))


This was required due to the change in sort.jl to a comprehension, which in turn was required because we were using the existence of a Slice to determine where a : was placed. There could be another workaround for that if this is deemed undesirable… but it seems quite nice to be able to create similarly offset arrays simply with [B[i,j] for (i,j) in axes(A)].

This moves in the opposite direction of what I'm doing in #25861 (narrowing the type-assertion). However, if we have to pick one the two, I guess this one should have priority.

Do we get the same benefit from ::AbstractArray{T,N}?

Also note that this change isn't completely predicated by the change in axis representation.

While it's not been tested, the introduction of this assertion broke things like [x for x in A] if A is an offset array. That had worked on 0.6.

Looks like with the explicit introduction of N, we need no type assertion at all 🎉 I've removed it from #25861, too.

This is an interesting case: it just falls out that unique across dimensions now preserves offset-ness of the non-uniqued dimensions

… default definition

This is the companion PR to JuliaLang/julia#27038.

mbauman · 2018-05-10T18:37:31Z

@timholy I'd love to hear your input here.

martinholters · 2018-05-14T10:02:43Z

Should we milestone this?

StefanKarpinski · 2018-05-14T19:11:39Z

Or put another way, I'm not sure why the triage label was removed without comment. It's generally best to at least leave a comment as to why something is being de-triaged.

mbauman · 2018-05-14T19:13:06Z

The consensus from triage was that it hadn't been open too long and it'd be nice to hear more feedback before making a final decision. Re-triaging this week makes sense.

andyferris · 2018-05-16T06:56:55Z

I haven't had a chance to delve into the details - but I love the stated properties and support the outcome. (BTW I think it was Tim that first pointed out to me that this property was desirable for Slice, but I can't dig up where).

Keno · 2018-05-17T19:02:59Z

Or put another way, I'm not sure why the triage label was removed without comment. It's generally best to at least leave a comment as to why something is being de-triaged.

Triage decision was to ask @timholy to review ;).

maleadt · 2018-05-18T07:28:37Z

@mbauman ... did you really intent to include a fax machine in your branch name? because that broke the JuliaGPU buildbot (the GitPoller to be specific, when ASCII encoding cmdline args to git fetch)
EDIT: easy enough fix over at the buildbot side, so don't worry about it 🙂

mbauman · 2018-05-18T14:55:57Z

Whoops, I'm sorry my bad pun caused extra work for you!

StefanKarpinski · 2018-05-24T20:05:30Z

Triage supports trying this out in 0.7-alpha and reverting before 1.0 if it's a disaster. Seems like it may be a big win but it's essentially impossible to know without trying.

martinholters · 2018-05-28T06:22:37Z

Is there anything here that needs to be done before merging?

ViralBShah · 2018-05-28T06:31:03Z

I have restarted Appveyor.

fredrikekre · 2018-05-29T07:40:01Z

This changed the output back to what it was some weeks ago (https://github.com/JuliaLang/julia/pull/27000/files#diff-89e97f67f8058d32eac8d917db5bbdbf) so broke some doctests

diff --git a/base/indices.jl b/base/indices.jl
index a05a7c0..06690a1 100644
--- a/base/indices.jl
+++ b/base/indices.jl
@@ -336,7 +336,7 @@ from cartesian to linear indexing:
 
 ```jldoctest
 julia> linear = LinearIndices((1:3, 1:2))
-LinearIndices{2,Tuple{UnitRange{Int64},UnitRange{Int64}}} with indices 1:3×1:2:
+3×2 LinearIndices{2,Tuple{UnitRange{Int64},UnitRange{Int64}}}:
  1  4
  2  5
  3  6
diff --git a/base/multidimensional.jl b/base/multidimensional.jl
index 4c390f7..78b3d4f 100644
--- a/base/multidimensional.jl
+++ b/base/multidimensional.jl
@@ -202,7 +202,7 @@ module IteratorsMD
 
     ```jldoctest
     julia> cartesian = CartesianIndices((1:3, 1:2))
-    CartesianIndices{2,Tuple{UnitRange{Int64},UnitRange{Int64}}} with indices 1:3×1:2:
+    3×2 CartesianIndices{2,Tuple{UnitRange{Int64},UnitRange{Int64}}}:
      CartesianIndex(1, 1)  CartesianIndex(1, 2)
      CartesianIndex(2, 1)  CartesianIndex(2, 2)
      CartesianIndex(3, 1)  CartesianIndex(3, 2)

KristofferC · 2018-05-29T07:55:03Z

CI ran 2 weeks ago here and when it has been that long, it is good to restart it imo.

JeffBezanson · 2018-05-29T16:26:31Z

Are there no tests for the changed printing? We should add some.

mbauman · 2018-05-29T16:38:11Z

Sure, I'll add a few more tests here. Looks like this will be fixed in #27294.

* Some more examples/refs for array docs * fix doctests from #27038 (comment)

This is the companion PR to JuliaLang/julia#27038.

* Make OffsetArray's axes offset themselves This is the companion PR to JuliaLang/julia#27038. * Display slices as simple ranges in show -- we allow constructing OffsetArrays in this manner * Add compatibility for 0.6

timholy · 2018-07-02T11:59:05Z

Sorry about being unresponsive when this was posted. This fixes some of the bugs I addressed in #21052, so I'll close that.

Long term, it's unfortunate that Slice does double-duty. https://github.com/JuliaArrays/IdentityRanges.jl splits the two concepts; if we wanted to, we could introduce an IdentityRange (also considered as a name was IdempotentRange, because r[r] === r) and then use Slice as a trivial wrapper that signals "whole dimension."

andyferris · 2018-07-02T12:39:18Z

+1, Tim.

WIP: use offset axes for offset arrays

925f224

mbauman added arrays [a, r, r, a, y, s] triage This should be discussed on a triage call labels May 9, 2018

WIP: more work on making axes offset-safe

0b17bd7

mbauman commented May 9, 2018

View reviewed changes

mbauman added the breaking This change will break code label May 9, 2018

mbauman added 8 commits May 9, 2018 18:11

Fix reshape(::OffsetArray, ...)

ff220c4

Better support for non-Int axes

3c43023

wip: more offsetarray fixes/tests

1b8bd34

Remove commented out code from failed brainstrom

31f6e9d

Fix offset unique test:

3b29af2

This is an interesting case: it just falls out that unique across dimensions now preserves offset-ness of the non-uniqued dimensions

Simplify similar(::Type{T}, ...) and require T<:AbstractArray for the…

5be3987

… default definition

Use the same extension system for reshape as similar.

c1ce85a

Fixup Offset similar(::Type, ...) definition to match

18ce47f

mbauman added a commit to mbauman/OffsetArrays.jl that referenced this pull request May 10, 2018

Make OffsetArray's axes offset themselves

d992593

This is the companion PR to JuliaLang/julia#27038.

mbauman mentioned this pull request May 10, 2018

Make OffsetArray's axes offset themselves JuliaArrays/OffsetArrays.jl#46

Merged

mbauman changed the title ~~WIP: use offset axes for offset arrays~~ RFC: use offset axes for offset arrays May 10, 2018

Keno removed the triage This should be discussed on a triage call label May 10, 2018

nalimilan mentioned this pull request May 14, 2018

Wrong IteratorSize for pairs(::IndexCartesian, ...) JuliaLang/Compat.jl#540

Closed

StefanKarpinski added the triage This should be discussed on a triage call label May 14, 2018

Merge branch 'master' into mb/axesofindicesofaxesofindiceso📠s

6c4b6dd

JeffBezanson added this to the 0.7 milestone May 28, 2018

JeffBezanson removed the triage This should be discussed on a triage call label May 28, 2018

JeffBezanson merged commit 8e6cfff into master May 29, 2018

JeffBezanson deleted the mb/axesofindicesofaxesofindiceso📠s branch May 29, 2018 03:37

fredrikekre added a commit to fredrikekre/julia that referenced this pull request May 29, 2018

fix doctests from JuliaLang#27038 (comment)

d267e1c

fredrikekre mentioned this pull request May 29, 2018

Strip the CartesianIndex wrapper from the ind2sub deprecation #25211

Merged

mbauman mentioned this pull request May 29, 2018

Some more examples/refs for array docs #27294

Merged

mbauman pushed a commit that referenced this pull request May 29, 2018

Some more examples/refs for array docs (#27294)

0c575eb

* Some more examples/refs for array docs * fix doctests from #27038 (comment)

mbauman added a commit to mbauman/OffsetArrays.jl that referenced this pull request Jun 6, 2018

Make OffsetArray's axes offset themselves

a461476

This is the companion PR to JuliaLang/julia#27038.

mbauman mentioned this pull request Jun 20, 2018

RFC: Add support for keys(g::Generator) (fixes #27612) #27640

Closed

timholy mentioned this pull request Jul 2, 2018

Fixes, tests, & the future of Base.Slice #21052

Closed

timholy mentioned this pull request Jul 13, 2018

Fix a few offset indexing bugs #28100

Merged

KristofferC mentioned this pull request Jul 18, 2018

make simd loop over ranges perform better #28166

Merged

mbauman mentioned this pull request Aug 7, 2018

LinearIndices behavior change #28503

Closed

mbauman mentioned this pull request Jan 13, 2023

getindex on an OffsetArray behaves unintuitively JuliaArrays/OffsetArrays.jl#322

Closed

mcabbott mentioned this pull request Feb 20, 2023

Base.stack fails if DimArray and non-DimArray mixed rafaqz/DimensionalData.jl#464

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

RFC: use offset axes for offset arrays #27038

RFC: use offset axes for offset arrays #27038

mbauman commented May 9, 2018 •

edited

Loading

mbauman May 9, 2018

martinholters May 10, 2018

mbauman May 10, 2018

mbauman May 10, 2018

martinholters May 14, 2018

mbauman commented May 10, 2018

martinholters commented May 14, 2018

StefanKarpinski commented May 14, 2018

mbauman commented May 14, 2018

andyferris commented May 16, 2018

Keno commented May 17, 2018

maleadt commented May 18, 2018 •

edited

Loading

mbauman commented May 18, 2018

StefanKarpinski commented May 24, 2018 •

edited

Loading

martinholters commented May 28, 2018

ViralBShah commented May 28, 2018

fredrikekre commented May 29, 2018

KristofferC commented May 29, 2018

JeffBezanson commented May 29, 2018

mbauman commented May 29, 2018

timholy commented Jul 2, 2018

andyferris commented Jul 2, 2018 •

edited

Loading

RFC: use offset axes for offset arrays #27038

RFC: use offset axes for offset arrays #27038

Conversation

mbauman commented May 9, 2018 • edited Loading

mbauman May 9, 2018

Choose a reason for hiding this comment

martinholters May 10, 2018

Choose a reason for hiding this comment

mbauman May 10, 2018

Choose a reason for hiding this comment

mbauman May 10, 2018

Choose a reason for hiding this comment

martinholters May 14, 2018

Choose a reason for hiding this comment

mbauman commented May 10, 2018

martinholters commented May 14, 2018

StefanKarpinski commented May 14, 2018

mbauman commented May 14, 2018

andyferris commented May 16, 2018

Keno commented May 17, 2018

maleadt commented May 18, 2018 • edited Loading

mbauman commented May 18, 2018

StefanKarpinski commented May 24, 2018 • edited Loading

martinholters commented May 28, 2018

ViralBShah commented May 28, 2018

fredrikekre commented May 29, 2018

KristofferC commented May 29, 2018

JeffBezanson commented May 29, 2018

mbauman commented May 29, 2018

timholy commented Jul 2, 2018

andyferris commented Jul 2, 2018 • edited Loading

mbauman commented May 9, 2018 •

edited

Loading

maleadt commented May 18, 2018 •

edited

Loading

StefanKarpinski commented May 24, 2018 •

edited

Loading

andyferris commented Jul 2, 2018 •

edited

Loading