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

implement promote for BitArray #44096

Merged
merged 2 commits into from
Mar 2, 2022
Merged

implement promote for BitArray #44096

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
c6e3146
fix reduced_indices type bug (#44096)
vtjnash Feb 22, 2022
5cac5b9
implement promote for BitArray (#44096)
vtjnash Feb 9, 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
20 changes: 13 additions & 7 deletions base/indices.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -351,6 +351,8 @@ struct Slice{T<:AbstractUnitRange} <: AbstractUnitRange{Int}
indices::T
end
Slice(S::Slice) = S
Slice{T}(S::Slice) where {T<:AbstractUnitRange} = Slice{T}(T(S.indices))

axes(S::Slice) = (IdentityUnitRange(S.indices),)
axes1(S::Slice) = IdentityUnitRange(S.indices)
axes(S::Slice{<:OneTo}) = (S.indices,)
Expand All @@ -366,7 +368,6 @@ getindex(S::Slice, i::StepRange{<:Integer}) = (@inline; @boundscheck checkbounds
show(io::IO, r::Slice) = print(io, "Base.Slice(", r.indices, ")")
iterate(S::Slice, s...) = iterate(S.indices, s...)


"""
IdentityUnitRange(range::AbstractUnitRange)

Expand All @@ -378,6 +379,8 @@ struct IdentityUnitRange{T<:AbstractUnitRange} <: AbstractUnitRange{Int}
indices::T
end
IdentityUnitRange(S::IdentityUnitRange) = S
IdentityUnitRange{T}(S::IdentityUnitRange) where {T<:AbstractUnitRange} = IdentityUnitRange{T}(T(S.indices))

# IdentityUnitRanges are offset and thus have offset axes, so they are their own axes
axes(S::IdentityUnitRange) = (S,)
axes1(S::IdentityUnitRange) = S
Expand Down Expand Up @@ -448,6 +451,8 @@ julia> linear[1,2]
struct LinearIndices{N,R<:NTuple{N,AbstractUnitRange{Int}}} <: AbstractArray{Int,N}
indices::R
end
convert(::Type{LinearIndices{N,R}}, inds::LinearIndices{N}) where {N,R<:NTuple{N,AbstractUnitRange{Int}}} =
LinearIndices{N,R}(convert(R, inds.indices))

LinearIndices(::Tuple{}) = LinearIndices{0,typeof(())}(())
LinearIndices(inds::NTuple{N,AbstractUnitRange{<:Integer}}) where {N} =
Expand All @@ -459,16 +464,17 @@ LinearIndices(A::Union{AbstractArray,SimpleVector}) = LinearIndices(axes(A))
_convert2ind(i::Integer) = Base.OneTo(i)
_convert2ind(ind::AbstractUnitRange) = first(ind):last(ind)

promote_rule(::Type{LinearIndices{N,R1}}, ::Type{LinearIndices{N,R2}}) where {N,R1,R2} =
LinearIndices{N,indices_promote_type(R1,R2)}

function indices_promote_type(::Type{Tuple{R1,Vararg{R1,N}}}, ::Type{Tuple{R2,Vararg{R2,N}}}) where {R1,R2,N}
R = promote_type(R1, R2)
Tuple{R,Vararg{R,N}}
return Tuple{R, Vararg{R, N}}
end

convert(::Type{LinearIndices{N,R}}, inds::LinearIndices{N}) where {N,R} =
LinearIndices(convert(R, inds.indices))
promote_rule(::Type{LinearIndices{N,R1}}, ::Type{LinearIndices{N,R2}}) where {N,R1,R2} =
LinearIndices{N,indices_promote_type(R1,R2)}
promote_rule(a::Type{Slice{T1}}, b::Type{Slice{T2}}) where {T1,T2} =
el_same(promote_type(T1, T2), a, b)
promote_rule(a::Type{IdentityUnitRange{T1}}, b::Type{IdentityUnitRange{T2}}) where {T1,T2} =
el_same(promote_type(T1, T2), a, b)

# AbstractArray implementation
IndexStyle(::Type{<:LinearIndices}) = IndexLinear()
Expand Down
4 changes: 2 additions & 2 deletions base/promotion.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -303,9 +303,9 @@ it for new types as appropriate.
"""
function promote_rule end

promote_rule(::Type{<:Any}, ::Type{<:Any}) = Bottom
promote_rule(::Type, ::Type) = Bottom

promote_result(::Type{<:Any},::Type{<:Any},::Type{T},::Type{S}) where {T,S} = (@inline; promote_type(T,S))
promote_result(::Type,::Type,::Type{T},::Type{S}) where {T,S} = (@inline; promote_type(T,S))
# If no promote_rule is defined, both directions give Bottom. In that
# case use typejoin on the original types instead.
promote_result(::Type{T},::Type{S},::Type{Bottom},::Type{Bottom}) where {T,S} = (@inline; typejoin(T, S))
Expand Down
8 changes: 6 additions & 2 deletions base/range.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1263,13 +1263,17 @@ function -(r::LinRange)
LinRange{typeof(start)}(start, -r.stop, length(r))
end


# promote eltype if at least one container wouldn't change, otherwise join container types.
el_same(::Type{T}, a::Type{<:AbstractArray{T,n}}, b::Type{<:AbstractArray{T,n}}) where {T,n} = a
el_same(::Type{T}, a::Type{<:AbstractArray{T,n}}, b::Type{<:AbstractArray{T,n}}) where {T,n} = a # we assume a === b
el_same(::Type{T}, a::Type{<:AbstractArray{T,n}}, b::Type{<:AbstractArray{S,n}}) where {T,S,n} = a
el_same(::Type{T}, a::Type{<:AbstractArray{S,n}}, b::Type{<:AbstractArray{T,n}}) where {T,S,n} = b
el_same(::Type, a, b) = promote_typejoin(a, b)

promote_result(::Type{<:AbstractArray}, ::Type{<:AbstractArray}, ::Type{T}, ::Type{S}) where {T,S} = (@inline; promote_type(T,S))
promote_result(::Type{T}, ::Type{S}, ::Type{Bottom}, ::Type{Bottom}) where {T<:AbstractArray,S<:AbstractArray} = (@inline; promote_typejoin(T,S))
# If no promote_rule is defined, both directions give Bottom. In that case use typejoin on the eltypes instead and give Array as the container.
promote_result(::Type{<:AbstractArray{T,n}}, ::Type{<:AbstractArray{S,n}}, ::Type{Bottom}, ::Type{Bottom}) where {T,S,n} = (@inline; Array{promote_type(T,S),n})
Copy link
Member

Choose a reason for hiding this comment

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

@vtjnash Line 1275 breaks StaticArrays.jl's test on master.
After this PR

julia> [(@MArray [1;2;3]), (@SArray [1;2;3])]
2-element Vector{Vector{Int64}}:
 [1, 2, 3]
 [1, 2, 3]

It was a Vector{Any} before this PR.
for a in [array1, array2] is general pattern for test. So this change seems a little "break"?

Copy link
Sponsor Member Author

Choose a reason for hiding this comment

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

Test should not do that, that is an incorrect test. [a, b] is a general pattern for changing a and b to a common type. If you do not want that, you must specify the type of the array you want (usually Any[...])

Copy link
Member

Choose a reason for hiding this comment

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

FWIW CategoricalArrays tests also used this pattern. Actually this revealed the lack of promote_type methods for CategoricalArrays.

This has the potential to disrupt lots of tests, as things like e.g. [["b", "a"], ["b", missing]] will suddenly give two Vector{Union{Missing, String}} objects while previously the first one was Vector{String}. I'm not saying it shouldn't be done though but we have to be ready to notice some breakage.

Copy link
Sponsor Member Author

Choose a reason for hiding this comment

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

@KristofferC these are mostly the 3 lines that would be removed to change the behavior back, or changed to define a different combination of desired behaviors (for example, just removing the last rule so that it falls back to the promote_typejoin behavior defined on the previous line). The remaining parts of the PR are cleanup (e.g. to add missing constructors revealed by this change) that should still be valid.

You could also try to add a specific rule just for BitArray, as begun here, to just specifically keep the change requested in #43551: https://github.com/JuliaLang/julia/pull/43646/files#r781334697

Comment on lines +1272 to +1275
Copy link
Member

Choose a reason for hiding this comment

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

Why define these in range.jl? When I was looking for such methods I looked in promotion.jl and in abstractarray.jl and I couldn't find them.


promote_rule(a::Type{UnitRange{T1}}, b::Type{UnitRange{T2}}) where {T1,T2} =
el_same(promote_type(T1, T2), a, b)
UnitRange{T}(r::UnitRange{T}) where {T<:Real} = r
Expand Down
4 changes: 2 additions & 2 deletions base/reducedim.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ function reduced_indices0(inds::Indices{N}, d::Int) where N
end

function reduced_indices(inds::Indices{N}, region) where N
rinds = [inds...]
rinds = collect(inds)
for i in region
isa(i, Integer) || throw(ArgumentError("reduced dimension(s) must be integers"))
d = Int(i)
Expand All @@ -58,7 +58,7 @@ function reduced_indices(inds::Indices{N}, region) where N
end

function reduced_indices0(inds::Indices{N}, region) where N
rinds = [inds...]
rinds = collect(inds)
for i in region
isa(i, Integer) || throw(ArgumentError("reduced dimension(s) must be integers"))
d = Int(i)
Expand Down
16 changes: 15 additions & 1 deletion test/bitarray.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -98,6 +98,20 @@ end

timesofar("conversions")

@testset "Promotions for size $sz" for (sz, T) in allsizes
@test isequal(promote(falses(sz...), zeros(sz...)),
(zeros(sz...), zeros(sz...)))
@test isequal(promote(trues(sz...), ones(sz...)),
(ones(sz...), ones(sz...)))
ae = falses(1, sz...)
ex = (@test_throws ErrorException promote(ae, ones(sz...))).value
@test startswith(ex.msg, "promotion of types Bit")
ex = (@test_throws ErrorException promote(ae, falses(sz...))).value
@test startswith(ex.msg, "promotion of types Bit")
end

timesofar("promotions")

@testset "utility functions" begin
b1 = bitrand(v1)
@test isequal(fill!(b1, true), trues(size(b1)))
Expand Down Expand Up @@ -1767,4 +1781,4 @@ end
@test all(bitarray[rangeout, rangein] .== true)
@test all(bitarray[rangein, rangeout] .== true)
end
end
end
15 changes: 9 additions & 6 deletions test/broadcast.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -691,16 +691,19 @@ end
@test a == [1 1; 2 2; 3 3]
end

@testset "scalar .=" begin
A = [[1,2,3],4:5,6]
@testset "scalar .= and promotion" begin
A = [[1, 2, 3], 4:5, 6]
@test A isa Vector{Any}
A[1] .= 0
@test A[1] == [0,0,0]
@test A[1] == [0, 0, 0]
@test_throws Base.CanonicalIndexError A[2] .= 0
@test_throws MethodError A[3] .= 0
A = [[1,2,3],4:5]
A = [[1, 2, 3], 4:5]
@test A isa Vector{Vector{Int}}
A[1] .= 0
@test A[1] == [0,0,0]
@test_throws Base.CanonicalIndexError A[2] .= 0
A[2] .= 0
@test A[1] == [0, 0, 0]
@test A[2] == [0, 0]
end

# Issue #22180
Expand Down