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.

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

Change in AbstractArray promotion #45511

Closed
nalimilan opened this issue May 30, 2022 · 7 comments · Fixed by #47893
Closed

Change in AbstractArray promotion #45511

nalimilan opened this issue May 30, 2022 · 7 comments · Fixed by #47893
Assignees
Labels
arrays [a, r, r, a, y, s] priority This should be addressed urgently regression Regression in behavior compared to a previous version
Milestone

Comments

@nalimilan
Copy link
Member

nalimilan commented May 30, 2022

#44096 changed promotion rules for AbstractArray objects. In current Julia releases and 1.8, [A, B] preserves the type of arrays A and B. In current master, arrays are promoted to a common type if their types differ. This can give surprising results, as it seems relatively common (#44096 (comment)) to test different array types using loops such as:

for A in [[true, true], trues(2), [1, 1]]
    @test f(A) == ...
end

which now only tests Vector{Int} on three identical arrays.

Maybe this will mainly affect tests, as this kind of pattern is unlikely to be used in implementations. But it's somewhat pernicious as it doesn't make tests fail, but instead silently reduces test coverage. If it happens in real code, promotion can lead to allocating a copy of the inputs, which could dramatically increase memory usage (at the benefit of type stability), in particular if some of the inputs are sparse but one of them is dense.

This change doesn't seem to have been discussed, but it was raised recently on Slack so I figured it would be worth going through triage to ensure this is what we want before releasing 1.8.

Cc: @vtjnash, @KristofferC, @fredrikekre, @mbauman

@nalimilan nalimilan added arrays [a, r, r, a, y, s] triage This should be discussed on a triage call labels May 30, 2022
@jishnub
Copy link
Contributor

jishnub commented May 30, 2022

Related: #44939, which is technically a breakage if the promotion rules aren't already defined.

@fredrikekre
Copy link
Member

Duplicate of #44821

@fredrikekre fredrikekre marked this as a duplicate of #44821 May 30, 2022
@N5N3
Copy link
Member

N5N3 commented May 30, 2022

This promotion rule is valid only if all AbstractArrays could be be converted to a Array.
But on master, we prohibit such convertion from AbstractArray with mismatched axes.
Should we just replace the error with a warning, as AbstractRanges have Array{T,1}(r::AbstractRange{T}) where {T} = vcat(r) that supports all offset.

@nalimilan
Copy link
Member Author

Something to note is that we don't define promotion rules for other container types. For example:

julia> [Dict(1=>2), Dict(1.0=>2.0)]
2-element Vector{Dict}:
 Dict(1 => 2)
 Dict(1.0 => 2.0)

@oscardssmith oscardssmith added this to the 1.9 milestone Jun 9, 2022
@oscardssmith
Copy link
Member

Triage agrees that this change was bad and should not have happened (although we probably explicitly said it was good at the time)

@KristofferC
Copy link
Member

Here is another example:

julia> A1'
1×10 adjoint(::LinearInterpolation{Vector{Float32}, Vector{Float32}, true, Float32}) with eltype Float32:
 1.0  2.0  3.0  4.0  5.0  1.0  2.0  3.0  4.0  5.0

julia> A2'
1×10 adjoint(::LinearInterpolation{UnitRange{Int64}, UnitRange{Int64}, true, Int64}) with eltype Int64:
 1  2  3  4  5  1  2  3  4  5

julia> [A1, A2]
2-element Vector{Vector{Float32}}:
 [1.0, 2.0, 3.0, 4.0, 5.0, 1.0, 2.0, 3.0, 4.0, 5.0]
 [1.0, 2.0, 3.0, 4.0, 5.0, 1.0, 2.0, 3.0, 4.0, 5.0]

Ref SciML/DataInterpolations.jl#126

@oscardssmith oscardssmith added the regression Regression in behavior compared to a previous version label Oct 27, 2022
@oscardssmith oscardssmith added the priority This should be addressed urgently label Oct 27, 2022
@bkamins
Copy link
Member

bkamins commented Dec 4, 2022

A related issue of discrepancy between a comprehension and array literal (and Julia manual promises here they work in the same way):

julia> x = :x => Int[]
:x => Int64[]

julia> y = :y => Float64[]
:y => Float64[]

julia> [x, y]
2-element Vector{Pair{Symbol, Vector{Float64}}}:
 :x => []
 :y => []

julia> [p for p in (x, y)]
2-element Vector{Pair{Symbol}}:
 :x => Int64[]
 :y => Float64[]

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
arrays [a, r, r, a, y, s] priority This should be addressed urgently regression Regression in behavior compared to a previous version
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants