implement promote for BitArray #44096
Conversation
|
@JakobSachs this turned out to be more complicated than I thought. WDYT? @JeffBezanson Thought you might be interested to look at the new |
vtjnash
added
the
status:merge me
|
Seems good to me, tho i cant say i full understand the changes you made, regarding the indices. |
The result is type-asserted to be equal to the input, so we need to use a non-promoting function.
Defines a fallback promote_result for any AbstractArray, for the case when specific promote_rules are not defined for the specific array type. Add a few good promote_rules too. Fix #43551 Co-authored-by: Jakob Sachs <[email protected]>
| 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}) |
There was a problem hiding this comment.
@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"?
There was a problem hiding this comment.
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[...])
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
@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
The result is type-asserted to be equal to the input, so we need to use a non-promoting function.
Defines a fallback promote_result for any AbstractArray, for the case when specific promote_rules are not defined for the specific array type. Add a few good promote_rules too. Fix JuliaLang#43551 Co-authored-by: Jakob Sachs <[email protected]>
It turns out JuliaLang#44096 changes the default promote behavior of `AbstractArray` with the same `ndims`. Thus some Test in `Base` is "skipped" silently. This PR tries to re-activate them.
| 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}) |
There was a problem hiding this comment.
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.
A recent change on Julia master (JuliaLang/julia#44096) makes an array of arrays use more precise promotion, which ends up converting `CategoricalArray`s into `Vector`s.
A recent change on Julia master (JuliaLang/julia#44096) makes an array of arrays use more precise promotion, which ends up converting `CategoricalArray`s into `Vector`s.
A recent change on Julia master (JuliaLang/julia#44096) makes an array of arrays use more precise promotion, which ends up converting `CategoricalArray`s into `Vector`s.
A recent change on Julia master (JuliaLang/julia#44096) makes an array of arrays use more precise promotion, which ends up converting `CategoricalArray`s into `Vector`s.
Fix #43551
Replaces #43646 @JakobSachs following my comment there, based on your work