Skip to content

Fix nested IdOffsetRange constructor #185

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.

Sign up for GitHub

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

Merged
jishnub merged 6 commits into from
Jan 18, 2021
Merged

Fix nested IdOffsetRange constructor #185

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
a2ebf55
Fix nested IdOffsetRange constructor
jishnub Jan 13, 2021
425e86f
fix for 2D views
jishnub Jan 14, 2021
57f181c
fix for 1d views
jishnub Jan 14, 2021
d60f10c
fix compute_offset1
jishnub Jan 14, 2021
d2533bc
add type assertions to convert
jishnub Jan 18, 2021
7f4eecb
Fix comment
jishnub Jan 18, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion Project.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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"
Expand Down
2 changes: 2 additions & 0 deletions docs/make.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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"),
Expand Down
25 changes: 16 additions & 9 deletions src/axes.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,31 +78,38 @@ 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 <: IdOffsetRange.
=#
function IdOffsetRange{T,IdOffsetRange{T,I}}(r::IdOffsetRange{T,I}, offset::T) where {T<:Integer,I<:AbstractUnitRange{T}}
Copy link
Member

@timholy timholy Jan 17, 2021

Choose a reason for hiding this comment

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

Do you need this? AFAICT it does the same thing as the default. If this is for ambiguity resolution, perhaps add a comment?

Copy link
Member Author

@jishnub jishnub Jan 18, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

This is to avoid a stack overflow error in another method. I'll leave a comment.

On master:

julia> IdOffsetRange{Int, IdOffsetRange{Int, UnitRange{Int}}}(IdOffsetRange(2:3, 1), 1)
ERROR: StackOverflowError:
Stacktrace:
 [1] IdOffsetRange at [...]/OffsetArrays.jl/src/axes.jl:80 [inlined]
 [2] IdOffsetRange at [...]/OffsetArrays.jl/src/axes.jl:85 [inlined]
 [3] convert at ./range.jl:148 [inlined]
 [4] offset_coerce at [...]/OffsetArrays.jl/src/axes.jl:125 [inlined]
 [5] IdOffsetRange{Int64,IdOffsetRange{Int64,UnitRange{Int64}}}(::IdOffsetRange{Int64,UnitRange{Int64}}, ::Int64) at [...]/OffsetArrays.jl/src/axes.jl:98
 [6] IdOffsetRange{Int64,IdOffsetRange{Int64,UnitRange{Int64}}}(::IdOffsetRange{Int64,UnitRange{Int64}}, ::Int64) at [...]/OffsetArrays.jl/src/axes.jl:99 (repeats 79983 times)

This is fixed by this method.

The method called on master is IdOffsetRange{T,I}(::IdOffsetRange, offset::Integer) which tries to coerce the argument types, and call IdOffsetRange{T,I}(::I, ::T) within itself. This is presumably meant for cases like

julia> IdOffsetRange{Int, UnitRange{Int}}(IdOffsetRange(2:3, 1), 1)
OffsetArrays.IdOffsetRange(4:5)

however if the typevar I itself is an IdOffsetRange it ends up calling itself because the type signature is more specific. There might be a better way around this than adding this extra method, I'd just introduced this to avoid the overflow.

Copy link
Member

@timholy timholy Jan 18, 2021

Choose a reason for hiding this comment

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

Makes sense. Perhaps the argument-type coercion doesn't need to specialize on IdOffsetRange for the range? If that's not an easy fix, then just adding a brief comment would be a nice addition.

Copy link
Member Author

@jishnub jishnub Jan 18, 2021

Choose a reason for hiding this comment

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

I think the idea behind that method was to remove the IdOffsetRange wrapper and coerce the parent range, as there is another method for generic AbstractUnitRange arguments. Not quite sure of the performance impacts of removing this, perhaps one may look into it in another PR. As of now I have added a comment in 7f4eecb

new{T,IdOffsetRange{T,I}}(r, offset)
end
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, 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)
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
Expand All @@ -123,12 +130,12 @@ 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),)
@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
Expand Down Expand Up @@ -176,7 +183,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
Expand Down
2 changes: 1 addition & 1 deletion src/utils.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,4 +73,4 @@ function _checkindices(N::Integer, indices, label)
end

_unwrap(r::IdOffsetRange) = r.parent .+ r.offset
_unwrap(r::IdentityUnitRange) = r.indices
_unwrap(r::IdentityUnitRange) = r.indices
59 changes: 50 additions & 9 deletions test/runtests.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 =
Expand All @@ -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)
Expand All @@ -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]
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -866,6 +878,35 @@ 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),)

# 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
Expand Down