Make no_offset_view inferrable by timholy · Pull Request #189 · JuliaArrays/OffsetArrays.jl

timholy · 2021-01-17T17:13:17Z

This changes the implementation so that the axis values
do not affect the type of the output.

Technically this is a breaking change, but only at the level
of the returned types, not at the level of returned values.

This changes the implementation so that the axis *value*s do not affect the type of the output. Technically this is a breaking change, but only at the level of the returned types, not at the level of returned values.

codecov · 2021-01-17T17:15:46Z

Codecov Report

Merging #189 (dac2419) into master (df73941) will decrease coverage by 0.66%.
The diff coverage is 90.00%.

@@            Coverage Diff             @@
##           master     #189      +/-   ##
==========================================
- Coverage   98.92%   98.26%   -0.67%     
==========================================
  Files           5        5              
  Lines         279      288       +9     
==========================================
+ Hits          276      283       +7     
- Misses          3        5       +2

Impacted Files	Coverage Δ
src/utils.jl	`100.00% <ø> (ø)`
src/OffsetArrays.jl	`97.94% <84.61%> (-0.99%)`	⬇️
src/axes.jl	`100.00% <100.00%> (+2.08%)`	⬆️
src/precompile.jl	`92.85% <0.00%> (-7.15%)`	⬇️

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 df73941...dac2419. Read the comment docs.

timholy · 2021-01-17T17:32:43Z

Test failure is just a coverage failure. I'd recommend waiting on this until #185 is dealt with, and any other "non-new-feature" PRs. Then this might go along with any other new features (#187? #179?) for a 1.6 release.

jishnub · 2021-01-17T18:04:21Z

I've wondered before if one objective of no_offset_view was to reduce the number of wrappers around the parent? Otherwise type-stability is a good idea

timholy · 2021-01-17T18:59:34Z

I'm sure it is, but anyone who complains can specialize it for their array type. Conversely, we can never work around the instability.

timholy · 2021-01-19T19:32:09Z

Before merging, I plan to add one fallback: for unknown array types with Base.OneTo axes, just return the array. That should address most of #189 (comment).

johnnychen94

A good change, but I hope we could pend this PR for a few weeks to see if there are other breaking changes; as far as I know, Augmentor.jl is vulnerable to this change due to its over-verbose dispatches and tests.

Said that, if this PR does fix some known performance issue, please do merge and release it.

timholy · 2021-01-21T23:09:55Z

as far as I know, Augmentor.jl is vulnerable to this change due to its over-verbose dispatches and tests.

On 1.6, it seems to fail tests even on the master branch. Any further thoughts about this? There's no real urgency, but I wondered if the OneTo change here might circumvent Augmentor needing a fix.

jishnub · 2021-01-28T04:35:54Z

Perhaps AbstractUnitRanges may be transformed to other AbstractUnitRanges by no_offset_view? This should fix #198.

one choice could be

OffsetArrays.no_offset_view(a::AbstractUnitRange) = UnitRange(a)

timholy · 2021-01-28T08:53:15Z

Better:

OffsetArrays.no_offset_view(a::AbstractUnitRange) = first(a):last(a)

That's more explicit that it only looks at the values, not the indices.

jishnub · 2021-01-28T09:17:28Z

Wouldn't this be open to someone defining a custom integer type such that a:b returns Base.IdentityUnitRange(UnitRange(a,b))? UnitRanges on the other hand always use 1-based indexing.

jishnub · 2021-02-07T08:07:11Z

While we are at it, we might unwrap views of OffsetArrays as well. For example,

julia> a = reshape(1:25, 5, 5);

julia> sa = @view a[2:3, 2:3];

julia> osa = OffsetArray(sa, OffsetArrays.Origin(2));

julia> sosa = @view osa[:,2];

julia> ososa = OffsetArrays.no_offset_view(sosa)
2-element OffsetArray(view(OffsetArray(view(reshape(::UnitRange{Int64}, 5, 5), 2:3, 2:3), 2:3, 2:3), :, 2), 1:2) with eltype Int64 with indices 1:2:
 7
 8

julia> av = @view sa[:, 1];

julia> av == ososa
true

In this case no_offset_view(sosa) could have stripped off a few layers and returned av. I wonder if such an optimization is worth it?

Something like this for example might be added:

function no_offset_view(SOA::SubArray{<:Any,<:Any,<:OffsetArray,<:Tuple{Vararg{Union{Real,AbstractUnitRange}}}})
    indsSOA = Base.parentindices(SOA)
    OA = parent(SOA)
    inds = _removeoffset(indsSOA, OA.offsets)
    v = view(parent(OA), inds...)
    no_offset_view(v)
end

_removeoffset(i::Tuple{AbstractVector, Vararg{Any}}, o::Tuple{Int,Vararg{Int}}) = (first(i) .- first(o), _removeoffset(tail(i), tail(o))...)
_removeoffset(i::Tuple{Real, Vararg{Any}}, o::Tuple{Int, Vararg{Int}}) = (first(i) - first(o), _removeoffset(tail(i), tail(o))...)
_removeoffset(i::Tuple, o::Tuple{}) = i

in which case

julia> OffsetArrays.no_offset_view(sosa)
2-element view(reshape(::UnitRange{Int64}, 5, 5), 2:3, 2) with eltype Int64:
 7
 8

timholy · 2021-02-07T12:30:37Z

Wouldn't this be open to someone defining a custom integer type such that a:b returns Base.IdentityUnitRange(UnitRange(a,b))? UnitRanges on the other hand always use 1-based indexing.

I guess I was worried about whether UnitRange(Base.IdentityUnitRange(-1, 1)) should return -1:1 or an error. convert seems like it should error (they are not "equivalent", whatever that means 😄), but constructor-calls probably shouldn't. After all, Base.IdentityUnitRange doesn't complain. So upon reflection I like your version better.

Closes #198 Co-authored-by: Jishnu Bhattacharya <jishnub@users.noreply.github.com>

timholy · 2021-02-07T13:57:31Z

While we are at it, we might unwrap views of OffsetArrays as well

Good idea. I was a little bit worried about how far your suggestion delved into the internal representations of all types. What do you think about the more minimal version I just pushed? It doesn't doubly-unwrap like yours, but it does prevent the types from growing even more complex. As a bonus, it handles other types of shifted views, too (that's not piracy because we own no_offset_view). Demo:

julia> using OffsetArrays

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

julia> V = view(A, OffsetArrays.IdentityUnitRange(2:3), OffsetArrays.IdentityUnitRange(2:3))
2×2 view(reshape(::UnitRange{Int64}, 3, 4), :, :) with eltype Int64 with indices 2:3×2:3:
 5  8
 6  9

julia> typeof(V)
SubArray{Int64, 2, Base.ReshapedArray{Int64, 2, UnitRange{Int64}, Tuple{}}, Tuple{Base.IdentityUnitRange{UnitRange{Int64}}, Base.IdentityUnitRange{UnitRange{Int64}}}, false}

julia> OffsetArrays.no_offset_view(V)
2×2 view(reshape(::UnitRange{Int64}, 3, 4), 2:3, 2:3) with eltype Int64:
 5  8
 6  9

julia> typeof(OffsetArrays.no_offset_view(V))
SubArray{Int64, 2, Base.ReshapedArray{Int64, 2, UnitRange{Int64}, Tuple{}}, Tuple{UnitRange{Int64}, UnitRange{Int64}}, false}

julia> O = OffsetArray(A, -1:1, 0:3)
3×4 OffsetArray(reshape(::UnitRange{Int64}, 3, 4), -1:1, 0:3) with eltype Int64 with indices -1:1×0:3:
 1  4  7  10
 2  5  8  11
 3  6  9  12

julia> V = view(O, 0:1, 1:2)
2×2 view(OffsetArray(reshape(::UnitRange{Int64}, 3, 4), -1:1, 0:3), 0:1, 1:2) with eltype Int64:
 5  8
 6  9

julia> typeof(V)
SubArray{Int64, 2, OffsetMatrix{Int64, Base.ReshapedArray{Int64, 2, UnitRange{Int64}, Tuple{}}}, Tuple{UnitRange{Int64}, UnitRange{Int64}}, false}

julia> OffsetArrays.no_offset_view(V)
2×2 view(OffsetArray(reshape(::UnitRange{Int64}, 3, 4), -1:1, 0:3), 0:1, 1:2) with eltype Int64:
 5  8
 6  9

julia> typeof(OffsetArrays.no_offset_view(V))
SubArray{Int64, 2, OffsetMatrix{Int64, Base.ReshapedArray{Int64, 2, UnitRange{Int64}, Tuple{}}}, Tuple{UnitRange{Int64}, UnitRange{Int64}}, false}

julia> r1, r2 = OffsetArrays.IdOffsetRange(1:3, -2), OffsetArrays.IdentityUnitRange(2:3)
(OffsetArrays.IdOffsetRange(-1:1), Base.IdentityUnitRange(2:3))

julia> V = view(O, r1, r2)
3×2 view(OffsetArray(reshape(::UnitRange{Int64}, 3, 4), -1:1, 0:3), OffsetArrays.IdOffsetRange(-1:1), :) with eltype Int64 with indices OffsetArrays.IdOffsetRange(-1:1)×2:3:
 7  10
 8  11
 9  12

julia> typeof(V)
SubArray{Int64, 2, OffsetMatrix{Int64, Base.ReshapedArray{Int64, 2, UnitRange{Int64}, Tuple{}}}, Tuple{OffsetArrays.IdOffsetRange{Int64, UnitRange{Int64}}, Base.IdentityUnitRange{UnitRange{Int64}}}, false}

julia> OffsetArrays.no_offset_view(V)
3×2 view(OffsetArray(reshape(::UnitRange{Int64}, 3, 4), -1:1, 0:3), -1:1, 2:3) with eltype Int64:
 7  10
 8  11
 9  12

julia> typeof(OffsetArrays.no_offset_view(V))
SubArray{Int64, 2, OffsetMatrix{Int64, Base.ReshapedArray{Int64, 2, UnitRange{Int64}, Tuple{}}}, Tuple{UnitRange{Int64}, UnitRange{Int64}}, false}

To me this feels like a good place to be---I don't think we gain a lot by unwrapping the parent OffsetArray, and the risk is less generic handling of the axes.

timholy · 2021-02-07T13:59:15Z

So weird that codecov marks all of no_offset_view and its helpers as untested. Locally, every line of this diff is tested.

jishnub · 2021-02-08T08:54:52Z

While we are at it, we might unwrap views of OffsetArrays as well

Good idea. I was a little bit worried about how far your suggestion delved into the internal representations of all types. What do you think about the more minimal version I just pushed? It doesn't doubly-unwrap like yours, but it does prevent the types from growing even more complex. As a bonus, it handles other types of shifted views, too (that's not piracy because we own no_offset_view). Demo:

julia> using OffsetArrays

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

julia> V = view(A, OffsetArrays.IdentityUnitRange(2:3), OffsetArrays.IdentityUnitRange(2:3))
2×2 view(reshape(::UnitRange{Int64}, 3, 4), :, :) with eltype Int64 with indices 2:3×2:3:
 5  8
 6  9

julia> typeof(V)
SubArray{Int64, 2, Base.ReshapedArray{Int64, 2, UnitRange{Int64}, Tuple{}}, Tuple{Base.IdentityUnitRange{UnitRange{Int64}}, Base.IdentityUnitRange{UnitRange{Int64}}}, false}

julia> OffsetArrays.no_offset_view(V)
2×2 view(reshape(::UnitRange{Int64}, 3, 4), 2:3, 2:3) with eltype Int64:
 5  8
 6  9

julia> typeof(OffsetArrays.no_offset_view(V))
SubArray{Int64, 2, Base.ReshapedArray{Int64, 2, UnitRange{Int64}, Tuple{}}, Tuple{UnitRange{Int64}, UnitRange{Int64}}, false}

julia> O = OffsetArray(A, -1:1, 0:3)
3×4 OffsetArray(reshape(::UnitRange{Int64}, 3, 4), -1:1, 0:3) with eltype Int64 with indices -1:1×0:3:
 1  4  7  10
 2  5  8  11
 3  6  9  12

julia> V = view(O, 0:1, 1:2)
2×2 view(OffsetArray(reshape(::UnitRange{Int64}, 3, 4), -1:1, 0:3), 0:1, 1:2) with eltype Int64:
 5  8
 6  9

julia> typeof(V)
SubArray{Int64, 2, OffsetMatrix{Int64, Base.ReshapedArray{Int64, 2, UnitRange{Int64}, Tuple{}}}, Tuple{UnitRange{Int64}, UnitRange{Int64}}, false}

julia> OffsetArrays.no_offset_view(V)
2×2 view(OffsetArray(reshape(::UnitRange{Int64}, 3, 4), -1:1, 0:3), 0:1, 1:2) with eltype Int64:
 5  8
 6  9

julia> typeof(OffsetArrays.no_offset_view(V))
SubArray{Int64, 2, OffsetMatrix{Int64, Base.ReshapedArray{Int64, 2, UnitRange{Int64}, Tuple{}}}, Tuple{UnitRange{Int64}, UnitRange{Int64}}, false}

julia> r1, r2 = OffsetArrays.IdOffsetRange(1:3, -2), OffsetArrays.IdentityUnitRange(2:3)
(OffsetArrays.IdOffsetRange(-1:1), Base.IdentityUnitRange(2:3))

julia> V = view(O, r1, r2)
3×2 view(OffsetArray(reshape(::UnitRange{Int64}, 3, 4), -1:1, 0:3), OffsetArrays.IdOffsetRange(-1:1), :) with eltype Int64 with indices OffsetArrays.IdOffsetRange(-1:1)×2:3:
 7  10
 8  11
 9  12

julia> typeof(V)
SubArray{Int64, 2, OffsetMatrix{Int64, Base.ReshapedArray{Int64, 2, UnitRange{Int64}, Tuple{}}}, Tuple{OffsetArrays.IdOffsetRange{Int64, UnitRange{Int64}}, Base.IdentityUnitRange{UnitRange{Int64}}}, false}

julia> OffsetArrays.no_offset_view(V)
3×2 view(OffsetArray(reshape(::UnitRange{Int64}, 3, 4), -1:1, 0:3), -1:1, 2:3) with eltype Int64:
 7  10
 8  11
 9  12

julia> typeof(OffsetArrays.no_offset_view(V))
SubArray{Int64, 2, OffsetMatrix{Int64, Base.ReshapedArray{Int64, 2, UnitRange{Int64}, Tuple{}}}, Tuple{UnitRange{Int64}, UnitRange{Int64}}, false}

To me this feels like a good place to be---I don't think we gain a lot by unwrapping the parent OffsetArray, and the risk is less generic handling of the axes.

This seems like a good (and a pretty cool) solution. If there are specific performance enhancements possible we might look into them later.

jishnub · 2021-02-08T09:12:39Z

We might require some special handling of Base.Slice to avoid changing the IndexStyle:

julia> a = reshape(1:25, 5, 5);

julia> oa = OffsetArray(a, OffsetArrays.Origin(2));

julia> soa = @view oa[:, :];

julia> IndexStyle(soa)
IndexLinear()

julia> OffsetArrays.no_offset_view(soa) |> IndexStyle
IndexCartesian()

This might fix it:

no_offset_view(a::Base.Slice) = Base.Slice(UnitRange(a))

This changes the implementation so that the axis *value*s do not affect the type of the output. Technically this is a breaking change, but only at the level of the returned types, not at the level of returned values.

Closes #198 Co-authored-by: Jishnu Bhattacharya <jishnub@users.noreply.github.com>

Not introduced by this PR, but easily fixed

…setArrays.jl into teh/infer_no_offset_view

In 1.0 Slice doesn't cleanly mean "the whole axis", it's conflated with IdentityUnitRange.

timholy · 2021-02-09T11:23:06Z

OK to merge?

jishnub · 2021-02-09T12:17:29Z

Looks good to be merged

timholy · 2021-02-09T12:42:39Z

Thanks for all your help, @jishnub!

timholy · 2021-02-09T12:43:53Z

Should this be 1.6 or 1.5.4? It's not exactly a bugfix, but neither is it a new feature.

jishnub · 2021-02-09T12:45:33Z

Perhaps 1.6, as certain return types are altered

Make no_offset_array inferrable

1ae05df

This changes the implementation so that the axis *value*s do not affect the type of the output. Technically this is a breaking change, but only at the level of the returned types, not at the level of returned values.

johnnychen94 approved these changes Jan 20, 2021

View reviewed changes

timholy added 2 commits January 21, 2021 16:57

don't add wrapper for OneTo axes arrays

ab1d1a2

Improve test coverage

4bcf979

timholy mentioned this pull request Jan 22, 2021

Improve inferrability JuliaArrays/MosaicViews.jl#28

Merged

Support AbstractUnitRange & SubArrays

87f15e9

Closes #198 Co-authored-by: Jishnu Bhattacharya <jishnub@users.noreply.github.com>

timholy force-pushed the teh/infer_no_offset_view branch from 055d2f4 to 87f15e9 Compare February 7, 2021 13:49

timholy changed the title ~~Make no_offset_array inferrable~~ Make no_offset_view inferrable Feb 7, 2021

timholy and others added 7 commits February 9, 2021 02:58

Preserve IndexStyle for no_offset_views

a2c4fea

Make no_offset_array inferrable

a630f1e

This changes the implementation so that the axis *value*s do not affect the type of the output. Technically this is a breaking change, but only at the level of the returned types, not at the level of returned values.

don't add wrapper for OneTo axes arrays

5a53eb4

Improve test coverage

0d977a9

Support AbstractUnitRange & SubArrays

4c11c2d

Closes #198 Co-authored-by: Jishnu Bhattacharya <jishnub@users.noreply.github.com>

Preserve IndexStyle for no_offset_views

103a629

Fix Adapt ambiguity

5db24ac

Not introduced by this PR, but easily fixed

timholy added 2 commits February 9, 2021 03:29

Merge branch 'teh/infer_no_offset_view' of github.com:JuliaArrays/Off…

187f14b

…setArrays.jl into teh/infer_no_offset_view

Restrict Slice specialization to julia > 1.0

fb65354

In 1.0 Slice doesn't cleanly mean "the whole axis", it's conflated with IdentityUnitRange.

jishnub reviewed Feb 9, 2021

View reviewed changes

Comment thread src/OffsetArrays.jl

Only specialize for SubArray on Julia 1.1+

2ef6e53

jishnub reviewed Feb 9, 2021

View reviewed changes

Comment thread test/runtests.jl Outdated

test alsoJulia 1.0 indexstyle

dac2419

timholy merged commit 3b5b11b into master Feb 9, 2021

timholy deleted the teh/infer_no_offset_view branch February 9, 2021 12:42

Conversation

timholy commented Jan 17, 2021

Uh oh!

codecov Bot commented Jan 17, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

timholy commented Jan 17, 2021

Uh oh!

jishnub commented Jan 17, 2021

Uh oh!

timholy commented Jan 17, 2021

Uh oh!

timholy commented Jan 19, 2021

Uh oh!

johnnychen94 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

timholy commented Jan 21, 2021

Uh oh!

jishnub commented Jan 28, 2021

Uh oh!

timholy commented Jan 28, 2021

Uh oh!

jishnub commented Jan 28, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jishnub commented Feb 7, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

timholy commented Feb 7, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

timholy commented Feb 7, 2021

Uh oh!

timholy commented Feb 7, 2021

Uh oh!

jishnub commented Feb 8, 2021

Uh oh!

jishnub commented Feb 8, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

timholy commented Feb 9, 2021

Uh oh!

Uh oh!

jishnub commented Feb 9, 2021

Uh oh!

timholy commented Feb 9, 2021

Uh oh!

timholy commented Feb 9, 2021

Uh oh!

jishnub commented Feb 9, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

codecov Bot commented Jan 17, 2021 •

edited

Loading

johnnychen94 left a comment •

edited

Loading

jishnub commented Jan 28, 2021 •

edited

Loading

jishnub commented Feb 7, 2021 •

edited

Loading

timholy commented Feb 7, 2021 •

edited

Loading

jishnub commented Feb 8, 2021 •

edited

Loading