Skip to content

Allow the conversion of AbstractUnitRanges to OrdinalRanges #40038

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
merged 5 commits into from
Feb 22, 2022
Merged

Allow the conversion of AbstractUnitRanges to OrdinalRanges #40038

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
30fae4d
Specialize CartesianIndices for AbstractUnitRange axes
jishnub Mar 15, 2021
9f81f93
Convert AbstractUnitRange to OrdinalRange
jishnub Mar 15, 2021
a9c52df
Limit parameters in OrdinalRange{T1,T2}(::AbstractUnitRange)
jishnub Mar 15, 2021
76f4d03
Update base/range.jl
jishnub Feb 17, 2022
4760ba9
Update base/range.jl
vtjnash Feb 18, 2022
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
6 changes: 2 additions & 4 deletions base/range.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1252,10 +1252,8 @@ AbstractUnitRange{T}(r::AbstractUnitRange{T}) where {T} = r
AbstractUnitRange{T}(r::UnitRange) where {T} = UnitRange{T}(r)
AbstractUnitRange{T}(r::OneTo) where {T} = OneTo{T}(r)

OrdinalRange{T1, T2}(r::StepRange) where {T1, T2<: Integer} = StepRange{T1, T2}(r)
OrdinalRange{T1, T2}(r::AbstractUnitRange{T1}) where {T1, T2<:Integer} = r
OrdinalRange{T1, T2}(r::UnitRange) where {T1, T2<:Integer} = UnitRange{T1}(r)
OrdinalRange{T1, T2}(r::OneTo) where {T1, T2<:Integer} = OneTo{T1}(r)
Copy link
Member

@johnnychen94 johnnychen94 Mar 15, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

I think these methods are still needed:

julia> OrdinalRange{Int, Int8}(1:10)
1:10 # master
MethodError # current PR

We might need more tests to catch this.

Copy link
Member Author

@jishnub jishnub Mar 15, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

What should this produce? I don't think that the behavior on master is correct.

julia> OrdinalRange{Int, Int8}(1:10)
1:10

julia> OrdinalRange{Int, Int8}(1:10) |> step |> typeof
Int64

However the docstring of OrdinalRange states that OrdinalRange{T, S} has spacings of type S. The present behavior on master is not consistent with the docstring.

Since

julia> supertype(AbstractUnitRange)
OrdinalRange{T, T} where T

that is perhaps the conversion that makes sense.

Copy link
Member

Choose a reason for hiding this comment

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

Okay, that makes sense. Given that the previous 4 methods are newly added in Julia 1.6 only(#37829), I believe it's totally fine to revert that with OrdinalRange{T, T}(r::AbstractUnitRange) method.

OrdinalRange{T, S}(r::OrdinalRange) where {T, S} = StepRange{T, S}(r)
OrdinalRange{T, T}(r::AbstractUnitRange) where {T} = AbstractUnitRange{T}(r)

function promote_rule(::Type{StepRange{T1a,T1b}}, ::Type{StepRange{T2a,T2b}}) where {T1a,T1b,T2a,T2b}
Tb = promote_type(T1b, T2b)
Expand Down
11 changes: 10 additions & 1 deletion test/offsetarray.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -788,7 +788,7 @@ end
end
end

@testset "proper patition for non-1-indexed vector" begin
@testset "proper partition for non-1-indexed vector" begin
@test Iterators.partition(OffsetArray(1:10,10), 5) |> collect == [1:5,6:10] # OffsetVector
@test Iterators.partition(OffsetArray(collect(1:10),10), 5) |> collect == [1:5,6:10] # OffsetVector
@test Iterators.partition(OffsetArray(reshape(1:9,3,3), (3,3)), 5) |> collect == [1:5,6:9] #OffsetMatrix
Expand Down Expand Up @@ -822,3 +822,12 @@ end
@test (@view x[end, -y[end]])[] == 3
@test (@view x[y[end], end])[] == 4
end

@testset "CartesianIndices (issue #40035)" begin
A = OffsetArray(big(1):big(2), 0);
B = OffsetArray(1:2, 0);
# axes of an OffsetArray may be converted to an AbstractUnitRange,
# but the conversion to an OrdinalRange was not defined.
# this is fixed in #40038, so the evaluation of its CartesianIndices should work
@test CartesianIndices(A) == CartesianIndices(B)
end
3 changes: 3 additions & 0 deletions test/testhelpers/OffsetArrays.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,9 @@ function IdOffsetRange{T}(r::IdOffsetRange) where T<:Integer
end
IdOffsetRange(r::IdOffsetRange) = r

AbstractUnitRange{T}(r::IdOffsetRange{T}) where {T} = r
AbstractUnitRange{T}(r::IdOffsetRange) where {T} = IdOffsetRange{T}(r)

# TODO: uncomment these when Julia is ready
# # Conversion preserves both the values and the indexes, throwing an InexactError if this
# # is not possible.
Expand Down