Constrain UnitRanges to integer types #118
Conversation
Change UnitRange to UnitRange{<:Integer} in fill, zeros, ones, trues and falses,
keeping in mind that OffsetArrays accept only integral offsets.
Codecov Report
@@ Coverage Diff @@
## master #118 +/- ##
==========================================
- Coverage 83.69% 81.91% -1.79%
==========================================
Files 2 2
Lines 184 188 +4
==========================================
Hits 154 154
- Misses 30 34 +4
Continue to review full report at Codecov.
|
| @@ -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} = | |||
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
|
For the record, I think this is a very good change. I am a bit worried about the very point that @johnnychen94 raises. Have you played with it extensively on your own machine? Does it introduce any problems? |
johnnychen94
changed the title
|
This is potentially breaking if one defines a type that subtypes module CustomInt
struct MyInt <: Real
x :: Int
end
MyInt(m::MyInt) = m
Base.Int(m::MyInt) = m.x
Base.Integer(m::MyInt) = m.x
Base.:(<=)(m::MyInt, n::MyInt) = m.x <= n.x
Base.:(+)(m::MyInt, n::MyInt) = MyInt(m.x + n.x)
Base.:(-)(m::MyInt, n::MyInt) = MyInt(m.x - n.x)
Base.round(m::MyInt, ::RoundingMode) = m
Base.promote_rule(::Type{MyInt}, ::Type{Int}) = Int
endNow, on master: julia> r = UnitRange(CustomInt.MyInt(2), CustomInt.MyInt(3))
Main.CustomInt.MyInt(2):Main.CustomInt.MyInt(3)
julia> zeros(r)
2-element OffsetArray(::Array{Float64,1}, 2:3) with eltype Float64 with indices 2:3:
0.0
0.0while after this PR julia> r = UnitRange(CustomInt.MyInt(2), CustomInt.MyInt(3))
Main.CustomInt.MyInt(2):Main.CustomInt.MyInt(3)
julia> zeros(r)
ERROR: MethodError: no method matching zeros(::Type{Float64}, ::Tuple{UnitRange{Main.CustomInt.MyInt}})However this does seem to be a good direction to proceed in, as it limits the extent of type-piracy in this package. The question now is -- should we consider this for OffsetArrays v2.0? I'm not sure about invalidations and incremental compilations, and would like to hear more about it. |
Change
UnitRangetoUnitRange{<:Integer}in the function signature offill,zeros,ones,truesandfalses, keeping in mind thatOffsetArraysaccept only integer offsets.Fixes #117