Skip to content
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

Jump to bottom

Constrain UnitRanges to integer types #118

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Constrain UnitRanges to integer types #118

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
14 changes: 7 additions & 7 deletions src/OffsetArrays.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -125,15 +125,15 @@ function Base.similar(::Type{T}, shape::Tuple{OffsetAxis,Vararg{OffsetAxis}}) wh
OffsetArray(P, map(offset, axes(P), shape))
end

Base.fill(v, inds::NTuple{N, Union{Integer, AbstractUnitRange}}) where {N} =
Copy link
Member

@johnnychen94 johnnychen94 Jun 12, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Not sure if this is wanted, I believe the main reason for this loose type annotation is for consistency to the Base implementation

Although it seems not the case according to the test report, adding such constrain "might" introduce incremental compilations and method ambiguities when collaborating with other packages(not only Base), so I prefer to get a practical need first(e.g., when any particular downstream package raises such need), and then make such changes.

This will also allow other packages to define these methods for various other UnitRange types.

Do you have an example of this to see how things could be work together?

Copy link
Member Author

Choose a reason for hiding this comment

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

I see. I was thinking of OffsetArrays as a fallback type if arrays are indexed using types that are supersets of Integers, eg. HalfIntegers. For example, this is the behavior that I'm looking for

fill(x, UnitRange{HalfInt}, UnitRange{HalfInt}) -> Array indexed by HalfInts
fill(x, UnitRange{HalfInt}, UnitRange{Int}) -> Array indexed by HalfInts
fill(x, UnitRange{Int}, UnitRange{HalfInt}) -> Array indexed by HalfInts
fill(x, UnitRange{Int}, UnitRange{Int}) -> OffsetArray indexed by Ints (defined in OffsetArrays, external to the package)

This is an extension of the scheme followed in OffsetArrays, where

fill(x, UnitRange{Int}, UnitRange{Int}) -> OffsetArray indexed by Ints
fill(x, Int, UnitRange{Int}) -> OffsetArray indexed by Ints
fill(x, UnitRange{Int}, Int) -> OffsetArray indexed by Ints
fill(x, Int, Int) -> Array indexed by Ints (defined in Base, external to the package)

Getting this behavior to work requires me to dispatch on Tuple{Vararg{Union{UnitRange{HalfInt},UnitRange{Int}}}}. This however doesn't work as OffsetArrays dispatches on Tuple{UnitRange,Vararg{UnitRange}} which is more specific.

Base.fill(v, inds::NTuple{N, Union{Integer, AbstractUnitRange{<:Integer}}}) where {N} =
fill!(OffsetArray(Array{typeof(v), N}(undef, map(indexlength, inds)), map(indexoffset, inds)), v)
Base.zeros(::Type{T}, inds::NTuple{N, Union{Integer, AbstractUnitRange}}) where {T, N} =
Base.zeros(::Type{T}, inds::NTuple{N, Union{Integer, AbstractUnitRange{<:Integer}}}) where {T, N} =
fill!(OffsetArray(Array{T, N}(undef, map(indexlength, inds)), map(indexoffset, inds)), zero(T))
Base.ones(::Type{T}, inds::NTuple{N, Union{Integer, AbstractUnitRange}}) where {T, N} =
Base.ones(::Type{T}, inds::NTuple{N, Union{Integer, AbstractUnitRange{<:Integer}}}) where {T, N} =
fill!(OffsetArray(Array{T, N}(undef, map(indexlength, inds)), map(indexoffset, inds)), one(T))
Base.trues(inds::NTuple{N, Union{Integer, AbstractUnitRange}}) where {N} =
Base.trues(inds::NTuple{N, Union{Integer, AbstractUnitRange{<:Integer}}}) where {N} =
fill!(OffsetArray(BitArray{N}(undef, map(indexlength, inds)), map(indexoffset, inds)), true)
Base.falses(inds::NTuple{N, Union{Integer, AbstractUnitRange}}) where {N} =
Base.falses(inds::NTuple{N, Union{Integer, AbstractUnitRange{<:Integer}}}) where {N} =
fill!(OffsetArray(BitArray{N}(undef, map(indexlength, inds)), map(indexoffset, inds)), false)

## Indexing
Expand Down Expand Up @@ -209,9 +209,9 @@ Base.show(io::IO, ::MIME"text/plain", r::OffsetRange) = show(io, r)

### Convenience functions ###

Base.fill(x, inds::Tuple{UnitRange,Vararg{UnitRange}}) =
Base.fill(x, inds::Tuple{UnitRange{<:Integer},Vararg{UnitRange{<:Integer}}}) =
fill!(OffsetArray{typeof(x)}(undef, inds), x)
@inline Base.fill(x, ind1::UnitRange, inds::UnitRange...) = fill(x, (ind1, inds...))
@inline Base.fill(x, ind1::UnitRange{<:Integer}, inds::UnitRange{<:Integer}...) = fill(x, (ind1, inds...))


### Some mutating functions defined only for OffsetVector ###
Expand Down