From 10c08efebef58ee857ae73684816be08bcabb893 Mon Sep 17 00:00:00 2001 From: Jishnu Bhattacharya Date: Thu, 20 Mar 2025 09:32:18 +0530 Subject: [PATCH 1/5] Fix no_offset_view for Slice --- src/OffsetArrays.jl | 1 - test/runtests.jl | 10 ++++++++-- 2 files changed, 8 insertions(+), 3 deletions(-) diff --git a/src/OffsetArrays.jl b/src/OffsetArrays.jl index e0b3381..8b0516f 100644 --- a/src/OffsetArrays.jl +++ b/src/OffsetArrays.jl @@ -694,7 +694,6 @@ no_offset_view(A::OffsetArray) = no_offset_view(parent(A)) if isdefined(Base, :IdentityUnitRange) # valid only if Slice is distinguished from IdentityUnitRange no_offset_view(a::Base.Slice{<:Base.OneTo}) = a - no_offset_view(a::Base.Slice) = Base.Slice(UnitRange(a)) no_offset_view(S::SubArray) = view(parent(S), map(no_offset_view, parentindices(S))...) end no_offset_view(a::Array) = a diff --git a/test/runtests.jl b/test/runtests.jl index b8d74b0..03dff94 100644 --- a/test/runtests.jl +++ b/test/runtests.jl @@ -2365,8 +2365,14 @@ Base.getindex(x::PointlessWrapper, i...) = x.parent[i...] V = view(O, r1, r2) @test V != collect(V) @test OffsetArrays.no_offset_view(V) == collect(V) - V = @view O[:,:] - @test IndexStyle(A) == IndexStyle(O) == IndexStyle(V) == IndexStyle(OffsetArrays.no_offset_view(V)) == IndexLinear() + # V = @view O[:,:] + # @test IndexStyle(A) == IndexStyle(O) == IndexStyle(V) == IndexStyle(OffsetArrays.no_offset_view(V)) == IndexLinear() + + @testset "issue #375" + arr = OffsetArray(reshape(1:15, 3, 5), 2, 3) + arr_no_offset = OffsetArrays.no_offset_view(@view arr[:, 4]) + @test all(!Base.has_offset_axes, axes(arr_no_offset)) + end end @testset "no nesting" begin From 1208d4dee27f780bbbca32f8f9ed52314a37171f Mon Sep 17 00:00:00 2001 From: Jishnu Bhattacharya Date: Thu, 20 Mar 2025 09:44:26 +0530 Subject: [PATCH 2/5] Fix testset begin --- test/runtests.jl | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/runtests.jl b/test/runtests.jl index 03dff94..189bbfe 100644 --- a/test/runtests.jl +++ b/test/runtests.jl @@ -2368,7 +2368,7 @@ Base.getindex(x::PointlessWrapper, i...) = x.parent[i...] # V = @view O[:,:] # @test IndexStyle(A) == IndexStyle(O) == IndexStyle(V) == IndexStyle(OffsetArrays.no_offset_view(V)) == IndexLinear() - @testset "issue #375" + @testset "issue #375" begin arr = OffsetArray(reshape(1:15, 3, 5), 2, 3) arr_no_offset = OffsetArrays.no_offset_view(@view arr[:, 4]) @test all(!Base.has_offset_axes, axes(arr_no_offset)) From c39f965fa9e0b9e293a7f9728dd5ce951def4f24 Mon Sep 17 00:00:00 2001 From: Jishnu Bhattacharya Date: Thu, 20 Mar 2025 09:51:17 +0530 Subject: [PATCH 3/5] Remove `Slice{<:OneTo}` specialization --- src/OffsetArrays.jl | 1 - 1 file changed, 1 deletion(-) diff --git a/src/OffsetArrays.jl b/src/OffsetArrays.jl index 8b0516f..32f71b0 100644 --- a/src/OffsetArrays.jl +++ b/src/OffsetArrays.jl @@ -693,7 +693,6 @@ julia> A no_offset_view(A::OffsetArray) = no_offset_view(parent(A)) if isdefined(Base, :IdentityUnitRange) # valid only if Slice is distinguished from IdentityUnitRange - no_offset_view(a::Base.Slice{<:Base.OneTo}) = a no_offset_view(S::SubArray) = view(parent(S), map(no_offset_view, parentindices(S))...) end no_offset_view(a::Array) = a From 96d0f15408aea7ea58bc9805697af0b2fe3ca0c4 Mon Sep 17 00:00:00 2001 From: Jishnu Bhattacharya Date: Thu, 20 Mar 2025 12:37:41 +0530 Subject: [PATCH 4/5] Shift parent and parentindices --- src/OffsetArrays.jl | 44 +++++++++++++++++++++++++++++++++++++++++++- test/runtests.jl | 4 ++-- 2 files changed, 45 insertions(+), 3 deletions(-) diff --git a/src/OffsetArrays.jl b/src/OffsetArrays.jl index 32f71b0..0913fde 100644 --- a/src/OffsetArrays.jl +++ b/src/OffsetArrays.jl @@ -693,7 +693,49 @@ julia> A no_offset_view(A::OffsetArray) = no_offset_view(parent(A)) if isdefined(Base, :IdentityUnitRange) # valid only if Slice is distinguished from IdentityUnitRange - no_offset_view(S::SubArray) = view(parent(S), map(no_offset_view, parentindices(S))...) + _onebasedslice(S::Base.Slice) = Base.Slice(Base.OneTo(length(S))) + _onebasedslice(S::Base.Slice{<:Base.OneTo}) = S + _onebasedslice(S) = S + _isoffsetslice(::Any) = false + _isoffsetslice(::Base.Slice) = true + _isoffsetslice(::Base.Slice{<:Base.OneTo}) = false + function no_offset_view(S::SubArray) + #= If a view contains an offset Slice axis, + i.e. it is a view of an offset array along the offset axis, + we shift the axis to a 1-based one. + E.g. Slice(2:3) -> Slice(Base.OneTo(2)) + We transform the `parent` as well as the `parentindices`, + so that the view still points to the same elements, even though the indices have changed. + This way, we retain the axis of the view as a `Slice` + =# + P = parent(S) + pinds = parentindices(S) + #= + Check if all the axes are `Slice`s and the parent has `OneTo` axes, + in which case we may unwrap the `OffsetArray` and forward the view to the parent. + =# + may_pop_parent = all(_isoffsetslice, pinds) && P isa OffsetArray && all(x -> x isa Base.OneTo, axes(parent(P))) + if may_pop_parent + return no_offset_view(P) + end + #= + we convert offset `Slice`s to 1-based ones using `_onebasedslice`. + `no_offset_view` on a `Slice{<:OneTo}` is a no-op, + while it converts the offset axes to 1-based ones. + The difference between `_onebasedslice` and `no_offset_view` is that + the latter does not change the value of the range, while the former does. + =# + newviewinds = map(no_offset_view ∘ _onebasedslice, pinds) + needs_shifting = any(_isoffsetslice, pinds) + P_maybeshiftedinds = if needs_shifting + t = Origin(parent(S)).index + neworigin = ntuple(i -> _isoffsetslice(pinds[i]) ? 1 : t[i], length(t)) + Origin(neworigin)(P) + else + P + end + view(P_maybeshiftedinds, newviewinds...) + end end no_offset_view(a::Array) = a no_offset_view(i::Number) = i diff --git a/test/runtests.jl b/test/runtests.jl index 189bbfe..c1a272e 100644 --- a/test/runtests.jl +++ b/test/runtests.jl @@ -2365,8 +2365,8 @@ Base.getindex(x::PointlessWrapper, i...) = x.parent[i...] V = view(O, r1, r2) @test V != collect(V) @test OffsetArrays.no_offset_view(V) == collect(V) - # V = @view O[:,:] - # @test IndexStyle(A) == IndexStyle(O) == IndexStyle(V) == IndexStyle(OffsetArrays.no_offset_view(V)) == IndexLinear() + V = @view O[:,:] + @test IndexStyle(A) == IndexStyle(O) == IndexStyle(V) == IndexStyle(OffsetArrays.no_offset_view(V)) == IndexLinear() @testset "issue #375" begin arr = OffsetArray(reshape(1:15, 3, 5), 2, 3) From 284625da0b7bc94587995775fa155937e40b55f4 Mon Sep 17 00:00:00 2001 From: Jishnu Bhattacharya Date: Thu, 20 Mar 2025 14:00:10 +0530 Subject: [PATCH 5/5] Simplify _no_offset_view dispatch --- src/OffsetArrays.jl | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/src/OffsetArrays.jl b/src/OffsetArrays.jl index 0913fde..9aa9f58 100644 --- a/src/OffsetArrays.jl +++ b/src/OffsetArrays.jl @@ -720,8 +720,10 @@ if isdefined(Base, :IdentityUnitRange) end #= we convert offset `Slice`s to 1-based ones using `_onebasedslice`. - `no_offset_view` on a `Slice{<:OneTo}` is a no-op, + The next call, `no_offset_view`, is a no-op on a `Slice{<:OneTo}`, while it converts the offset axes to 1-based ones. + Eventually, we end up with a `Tuple` comprising `Slice{<:OneTo}`s and other 1-based axes. + The difference between `_onebasedslice` and `no_offset_view` is that the latter does not change the value of the range, while the former does. =# @@ -742,10 +744,9 @@ no_offset_view(i::Number) = i no_offset_view(A::AbstractArray) = _no_offset_view(axes(A), A) _no_offset_view(::Tuple{}, A::AbstractArray{T,0}) where T = A _no_offset_view(::Tuple{Base.OneTo, Vararg{Base.OneTo}}, A::AbstractArray) = A -# the following method is needed for ambiguity resolution -_no_offset_view(::Tuple{Base.OneTo, Vararg{Base.OneTo}}, A::AbstractUnitRange) = A -_no_offset_view(::Any, A::AbstractArray) = OffsetArray(A, Origin(1)) -_no_offset_view(::Any, A::AbstractUnitRange) = UnitRange(A) +_no_offset_view(::Any, A::AbstractArray) = _no_offset_view(A) +_no_offset_view(A::AbstractArray) = OffsetArray(A, Origin(1)) +_no_offset_view(A::AbstractUnitRange) = UnitRange(A) ##### # center/centered