From a2ebf552296ea2de00b225d59688981ee05e0aa1 Mon Sep 17 00:00:00 2001 From: jishnub Date: Wed, 13 Jan 2021 19:25:08 +0400 Subject: [PATCH 1/5] Fix nested IdOffsetRange constructor --- Project.toml | 2 +- docs/make.jl | 2 ++ src/axes.jl | 6 ++++-- src/utils.jl | 2 +- test/runtests.jl | 40 +++++++++++++++++++++++++++++++--------- 5 files changed, 39 insertions(+), 13 deletions(-) diff --git a/Project.toml b/Project.toml index f82f73e2..6b287db0 100644 --- a/Project.toml +++ b/Project.toml @@ -1,6 +1,6 @@ name = "OffsetArrays" uuid = "6fe1bfb0-de20-5000-8ca7-80f57d26f881" -version = "1.5.0" +version = "1.5.1" [deps] Adapt = "79e6a3ab-5dfb-504d-930d-738a2a938a0e" diff --git a/docs/make.jl b/docs/make.jl index 7a769a95..ce7a68cd 100644 --- a/docs/make.jl +++ b/docs/make.jl @@ -1,6 +1,8 @@ using Documenter, JSON using OffsetArrays +DocMeta.setdocmeta!(OffsetArrays, :DocTestSetup, :(using OffsetArrays); recursive=true) + makedocs( sitename = "OffsetArrays", format = Documenter.HTML(prettyurls = get(ENV, "CI", nothing) == "true"), diff --git a/src/axes.jl b/src/axes.jl index dcb65240..ef03bd2a 100644 --- a/src/axes.jl +++ b/src/axes.jl @@ -78,6 +78,9 @@ struct IdOffsetRange{T<:Integer,I<:AbstractUnitRange{T}} <: AbstractUnitRange{T} offset::T IdOffsetRange{T,I}(r::I, offset::T) where {T<:Integer,I<:AbstractUnitRange{T}} = new{T,I}(r, offset) + function IdOffsetRange{T,IdOffsetRange{T,I}}(r::IdOffsetRange{T,I}, offset::T) where {T<:Integer,I<:AbstractUnitRange{T}} + new{T,IdOffsetRange{T,I}}(r, offset) + end end # Construction/coercion from arbitrary AbstractUnitRanges @@ -96,13 +99,12 @@ IdOffsetRange(r::AbstractUnitRange{T}, offset::Integer = 0) where T<:Integer = IdOffsetRange{T,I}(r::IdOffsetRange{T,I}) where {T<:Integer,I<:AbstractUnitRange{T}} = r function IdOffsetRange{T,I}(r::IdOffsetRange, offset::Integer = 0) where {T<:Integer,I<:AbstractUnitRange{T}} rc, offset_rc = offset_coerce(I, r.parent) - return IdOffsetRange{T,I}(rc, r.offset + offset + offset_rc) + return IdOffsetRange{T,I}(rc, convert(T, r.offset + offset + offset_rc)) end function IdOffsetRange{T}(r::IdOffsetRange, offset::Integer = 0) where T<:Integer return IdOffsetRange{T}(r.parent, r.offset + offset) end IdOffsetRange(r::IdOffsetRange) = r -IdOffsetRange(r::IdOffsetRange, offset::Integer) = typeof(r)(r.parent, offset + r.offset) # TODO: uncomment these when Julia is ready # # Conversion preserves both the values and the indexes, throwing an InexactError if this diff --git a/src/utils.jl b/src/utils.jl index 72414804..2d9e8453 100644 --- a/src/utils.jl +++ b/src/utils.jl @@ -73,4 +73,4 @@ function _checkindices(N::Integer, indices, label) end _unwrap(r::IdOffsetRange) = r.parent .+ r.offset -_unwrap(r::IdentityUnitRange) = r.indices \ No newline at end of file +_unwrap(r::IdentityUnitRange) = r.indices diff --git a/test/runtests.jl b/test/runtests.jl index 70f19a16..ccbd513f 100644 --- a/test/runtests.jl +++ b/test/runtests.jl @@ -9,6 +9,8 @@ using EllipsisNotation using Adapt using StaticArrays +DocMeta.setdocmeta!(OffsetArrays, :DocTestSetup, :(using OffsetArrays); recursive=true) + # https://github.com/JuliaLang/julia/pull/29440 if VERSION < v"1.1.0-DEV.389" Base.:(:)(I::CartesianIndex{N}, J::CartesianIndex{N}) where N = @@ -22,6 +24,14 @@ struct TupleOfRanges{N} x ::NTuple{N, UnitRange{Int}} end +function same_value(r1, r2) + length(r1) == length(r2) || return false + for (v1, v2) in zip(r1, r2) + v1 == v2 || return false + end + return true +end + @testset "Project meta quality checks" begin # Not checking compat section for test-only dependencies Aqua.test_all(OffsetArrays; project_extras=true, deps_compat=true, stale_deps=true, project_toml_formatting=true) @@ -31,13 +41,7 @@ end end @testset "IdOffsetRange" begin - function same_value(r1, r2) - length(r1) == length(r2) || return false - for (v1, v2) in zip(r1, r2) - v1 == v2 || return false - end - return true - end + function check_indexed_by(r, rindx) for i in rindx r[i] @@ -98,8 +102,16 @@ end @test same_value(r, 3:5) check_indexed_by(r, 3:5) - r = IdOffsetRange(IdOffsetRange(3:5, 2), 1) - @test parent(r) isa UnitRange + rp = Base.OneTo(3) + r = IdOffsetRange(rp) + r2 = IdOffsetRange{Int,typeof(r)}(r, 1) + @test same_value(r2, 2:4) + check_indexed_by(r2, 2:4) + + r2 = IdOffsetRange{Int32,IdOffsetRange{Int32,Base.OneTo{Int32}}}(r, 1) + @test typeof(r2) == IdOffsetRange{Int32,IdOffsetRange{Int32,Base.OneTo{Int32}}} + @test same_value(r2, 2:4) + check_indexed_by(r2, 2:4) # conversion preserves both the values and the axes, throwing an error if this is not possible @test @inferred(oftype(ro, ro)) === ro @@ -866,6 +878,16 @@ end @test S[0, 2, 2] == A[0, 4, 2] @test S[1, 1, 2] == A[1, 3, 2] @test axes(S) == (OffsetArrays.IdOffsetRange(0:1), Base.OneTo(2), OffsetArrays.IdOffsetRange(2:5)) + + # fix IdOffsetRange(::IdOffsetRange, offset) nesting from #178 + b = 1:20 + bov = OffsetArray(view(b, 3:4), 3:4) + c = @view b[bov] + @test same_value(c, 3:4) + @test axes(c,1) == 3:4 + d = OffsetArray(c, 1:2) + @test same_value(d, c) + @test axes(d,1) == 1:2 end @testset "iteration" begin From 425e86f5fef2886e387fbcac26e28f1aa97132fb Mon Sep 17 00:00:00 2001 From: jishnub Date: Thu, 14 Jan 2021 15:28:42 +0400 Subject: [PATCH 2/5] fix for 2D views --- src/axes.jl | 2 +- test/runtests.jl | 19 +++++++++++++++++++ 2 files changed, 20 insertions(+), 1 deletion(-) diff --git a/src/axes.jl b/src/axes.jl index dcb65240..23cf9615 100644 --- a/src/axes.jl +++ b/src/axes.jl @@ -128,7 +128,7 @@ offset_coerce(::Type{I}, r::AbstractUnitRange) where I<:AbstractUnitRange = @inline Base.parent(r::IdOffsetRange) = r.parent @inline Base.axes(r::IdOffsetRange) = (Base.axes1(r),) @inline Base.axes1(r::IdOffsetRange) = IdOffsetRange(Base.axes1(r.parent), r.offset) -@inline Base.unsafe_indices(r::IdOffsetRange) = (r,) +@inline Base.unsafe_indices(r::IdOffsetRange) = (Base.axes1(r),) @inline Base.length(r::IdOffsetRange) = length(r.parent) Base.reduced_index(i::IdOffsetRange) = typeof(i)(first(i):first(i)) # Workaround for #92 on Julia < 1.4 diff --git a/test/runtests.jl b/test/runtests.jl index 70f19a16..47b46a4b 100644 --- a/test/runtests.jl +++ b/test/runtests.jl @@ -866,6 +866,25 @@ end @test S[0, 2, 2] == A[0, 4, 2] @test S[1, 1, 2] == A[1, 3, 2] @test axes(S) == (OffsetArrays.IdOffsetRange(0:1), Base.OneTo(2), OffsetArrays.IdOffsetRange(2:5)) + + # issue #186 + a = reshape(1:12, 3, 4) + r = OffsetArrays.IdOffsetRange(3:4) + av = view(a, :, r) + @test av == a[:, 3:4] + @test axes(av) == (axes(a,1), axes(r,1)) + r = OffsetArrays.IdOffsetRange(1:2,2) + av = view(a, :, r) + @test no_offset_view(av) == a[:, 3:4] + @test axes(av) == (axes(a,1), axes(r,1)) + # r = OffsetArrays.IdOffsetRange(2:3) + # av1d = view(a, r, 3) + # @test av1d == a[2:3, 3] + # @test axes(av1d) == (axes(r,1),) + # r = OffsetArrays.IdOffsetRange(Base.OneTo(2), 1) + # av1d = view(a, r, 3) + # @test no_offset_view(av1d) == a[2:3, 3] + # @test axes(av1d) == (axes(r,1),) end @testset "iteration" begin From d60f10cdcd081eaf3a8172b175ec2d488ab6bc42 Mon Sep 17 00:00:00 2001 From: jishnub Date: Thu, 14 Jan 2021 15:58:11 +0400 Subject: [PATCH 3/5] fix compute_offset1 --- src/axes.jl | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/axes.jl b/src/axes.jl index d5ee7957..1227dce9 100644 --- a/src/axes.jl +++ b/src/axes.jl @@ -178,7 +178,7 @@ if VERSION < v"1.5.2" # issue 100, 133: IdOffsetRange as another index-preserving case shouldn't comtribute offsets # fixed by https://github.com/JuliaLang/julia/pull/37204 @inline Base.compute_offset1(parent, stride1::Integer, dims::Tuple{Int}, inds::Tuple{IdOffsetRange}, I::Tuple) = - Base.compute_linindex(parent, I) - stride1*first(inds[1]) + Base.compute_linindex(parent, I) - stride1*first(Base.axes1(inds[1])) end # This was deemed "too private" to extend: see issue #184 From d2533bcf157f925bae5b725fe530d710e19891d5 Mon Sep 17 00:00:00 2001 From: jishnub Date: Mon, 18 Jan 2021 10:22:28 +0400 Subject: [PATCH 4/5] add type assertions to convert --- src/axes.jl | 17 +++++++++++------ 1 file changed, 11 insertions(+), 6 deletions(-) diff --git a/src/axes.jl b/src/axes.jl index 1227dce9..6c540b87 100644 --- a/src/axes.jl +++ b/src/axes.jl @@ -78,6 +78,11 @@ struct IdOffsetRange{T<:Integer,I<:AbstractUnitRange{T}} <: AbstractUnitRange{T} offset::T IdOffsetRange{T,I}(r::I, offset::T) where {T<:Integer,I<:AbstractUnitRange{T}} = new{T,I}(r, offset) + + #= This method is necessary to avoid a StackOverflowError in IdOffsetRange{T,I}(r::IdOffsetRange, offset::Integer) + The type signature in that method is more specific than IdOffsetRange{T,I}(r::I, offset::T), + so it ends up calling itself if I is of type IdOffsetRange. + =# function IdOffsetRange{T,IdOffsetRange{T,I}}(r::IdOffsetRange{T,I}, offset::T) where {T<:Integer,I<:AbstractUnitRange{T}} new{T,IdOffsetRange{T,I}}(r, offset) end @@ -86,20 +91,20 @@ end # Construction/coercion from arbitrary AbstractUnitRanges function IdOffsetRange{T,I}(r::AbstractUnitRange, offset::Integer = 0) where {T<:Integer,I<:AbstractUnitRange{T}} rc, o = offset_coerce(I, r) - return IdOffsetRange{T,I}(rc, convert(T, o+offset)) + return IdOffsetRange{T,I}(rc, convert(T, o+offset)::T) end function IdOffsetRange{T}(r::AbstractUnitRange, offset::Integer = 0) where T<:Integer - rc = convert(AbstractUnitRange{T}, r) - return IdOffsetRange{T,typeof(rc)}(rc, convert(T, offset)) + rc = convert(AbstractUnitRange{T}, r)::AbstractUnitRange{T} + return IdOffsetRange{T,typeof(rc)}(rc, convert(T, offset)::T) end IdOffsetRange(r::AbstractUnitRange{T}, offset::Integer = 0) where T<:Integer = - IdOffsetRange{T,typeof(r)}(r, convert(T, offset)) + IdOffsetRange{T,typeof(r)}(r, convert(T, offset)::T) # Coercion from other IdOffsetRanges IdOffsetRange{T,I}(r::IdOffsetRange{T,I}) where {T<:Integer,I<:AbstractUnitRange{T}} = r function IdOffsetRange{T,I}(r::IdOffsetRange, offset::Integer = 0) where {T<:Integer,I<:AbstractUnitRange{T}} rc, offset_rc = offset_coerce(I, r.parent) - return IdOffsetRange{T,I}(rc, convert(T, r.offset + offset + offset_rc)) + return IdOffsetRange{T,I}(rc, convert(T, r.offset + offset + offset_rc)::T) end function IdOffsetRange{T}(r::IdOffsetRange, offset::Integer = 0) where T<:Integer return IdOffsetRange{T}(r.parent, r.offset + offset) @@ -125,7 +130,7 @@ end # Fallback, specialze this method if `convert(I, r)` doesn't do what you need offset_coerce(::Type{I}, r::AbstractUnitRange) where I<:AbstractUnitRange = - convert(I, r), 0 + convert(I, r)::I, 0 @inline Base.parent(r::IdOffsetRange) = r.parent @inline Base.axes(r::IdOffsetRange) = (Base.axes1(r),) From 7f4eecb7a66963374e1cc1a16f8626845ed3c1d1 Mon Sep 17 00:00:00 2001 From: jishnub Date: Mon, 18 Jan 2021 11:32:09 +0400 Subject: [PATCH 5/5] Fix comment --- src/axes.jl | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/axes.jl b/src/axes.jl index 6c540b87..d5e168e0 100644 --- a/src/axes.jl +++ b/src/axes.jl @@ -79,9 +79,9 @@ struct IdOffsetRange{T<:Integer,I<:AbstractUnitRange{T}} <: AbstractUnitRange{T} IdOffsetRange{T,I}(r::I, offset::T) where {T<:Integer,I<:AbstractUnitRange{T}} = new{T,I}(r, offset) - #= This method is necessary to avoid a StackOverflowError in IdOffsetRange{T,I}(r::IdOffsetRange, offset::Integer) + #= This method is necessary to avoid a StackOverflowError in IdOffsetRange{T,I}(r::IdOffsetRange, offset::Integer). The type signature in that method is more specific than IdOffsetRange{T,I}(r::I, offset::T), - so it ends up calling itself if I is of type IdOffsetRange. + so it ends up calling itself if I <: IdOffsetRange. =# function IdOffsetRange{T,IdOffsetRange{T,I}}(r::IdOffsetRange{T,I}, offset::T) where {T<:Integer,I<:AbstractUnitRange{T}} new{T,IdOffsetRange{T,I}}(r, offset)