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

Let Base handle concatenation of arrays and numbers #48933

Merged
merged 9 commits into from
Mar 10, 2023

Conversation

dkarrasch
Copy link
Member

@dkarrasch dkarrasch commented Mar 7, 2023

Fixes #48850.

julia> using BenchmarkTools

julia> x = 1; y = 2:2;

julia> @btime vcat($x, $x);
  33.696 ns (1 allocation: 80 bytes)

julia> @btime vcat($y, $y);
  40.178 ns (1 allocation: 80 bytes)

julia> @btime vcat($x, $y);
  43.897 ns (1 allocation: 80 bytes)

The issue was that in Base the concatenation methods are completely generic, and that the methods in uniformscaling.jl are pirating the case when only a vecormat and numbers are present. This resolves the ambiguity, and simplifies the case where both uniform scalings and numbers are present. In that case, the scalings can only have size 1x1, or, equivalently, be numbers.

@dkarrasch dkarrasch added linear algebra Linear algebra arrays [a, r, r, a, y, s] labels Mar 7, 2023
@dkarrasch
Copy link
Member Author

Honestly, I believe we should backport this to v1.9, not sure if v1.8 is still necessary. @KristofferC Do you think this is feasible?

@mikmoore
Copy link
Contributor

mikmoore commented Mar 7, 2023

Another possibility might be to change the existing UniformScaling methods to use map(szfun,A) where szfun(::UniformScaling) = -1 and szfun(x::Any) = size(x,$dim) for the appropriate dimension. I.e., the same as it is now, but use a map over the inhomogeneous A::Tuple instead of a loop over it. Then all the unstable loops over A could be changed to loops over the resulting NTuple{N,Int}. This would resolve the instability even when UniformScaling are present.

@dkarrasch
Copy link
Member Author

I'm running local tests to see if my adoption of your proposal works. I'd like to keep the new methods in abstractarray.jl because that removes the piracy by LinearAlgebra.

@dkarrasch
Copy link
Member Author

I'm getting poor benchmarks with the changes in uniformscaling.jl. Let's keep the fix to its absolute minimum. It resolves the piracy that exists since v1.8. Performance optimizations can come later in a different PR, perhaps using the following quick-and-dirty benchmarks, extracted from the tests:

using LinearAlgebra, BenchmarkTools

A = rand(3,4)
B = rand(3,3)
C = rand(0,3)
D = rand(2,0)
E = rand(1,3)
F = rand(3,1)
α = rand()

begin
    @btime hcat($A, $(2I));
    @btime hcat($E, $α);
    @btime hcat($A, $(2I));
    @btime hcat($E, $α);
    @btime hcat($E, $α, 2I);
    @btime vcat($A, $(2I));
    @btime vcat($F, $α);
    @btime vcat($F, $α, $(2I));
    @btime hcat($C, $(2I));
    @btime vcat($D, $(2I));
    @btime hcat($I, $(3I), $A, $(2I));
    @btime vcat($I, $(3I), $A, $(2I));
    @btime hvcat((2,1,2), $B, $(2I), I, $(3I), $(4I));
    @btime hvcat((3,1), $C, $C, I, $(3I));
    @btime hvcat((2,2,2), $I, $(2I), $(3I), $(4I), $C, $C);
    @btime hvcat((2,2,4), $C, $C, I, $(2I), $(3I), $(4I), $(5I), $D);
    @btime hvcat((2,3,2), $B, $(2I), $C, $C, I, $(3I), $(4I));
    @btime hvcat((3,2,1), $C, $C, I, $B, $(3I), $(2I));
    @btime hvcat((1,2), $A, $E, $α);
    @btime hvcat((2,2), $α, $E, $F, $(3I));
    @btime hvcat((2,2), $(3I), $F, $E, $α);
end

@dkarrasch dkarrasch added merge me PR is reviewed. Merge when all tests are passing backport 1.8 Change should be backported to release-1.8 backport 1.9 Change should be backported to release-1.9 labels Mar 8, 2023
@dkarrasch dkarrasch closed this Mar 8, 2023
@dkarrasch dkarrasch reopened this Mar 8, 2023
@dkarrasch
Copy link
Member Author

Test failure is unrelated. I added one safe optimization, that is orthogonal to your suggestions @mikmoore and which yields significant speedup. We should look at the code for concatenation of Union{AbstractVecOrMat,UniformScaling,Number} separately.

@staticfloat staticfloat merged commit e6c84a1 into master Mar 10, 2023
@staticfloat staticfloat deleted the dk/cat_array_number branch March 10, 2023 06:21
@giordano giordano removed the merge me PR is reviewed. Merge when all tests are passing label Mar 12, 2023
@KristofferC KristofferC mentioned this pull request Mar 24, 2023
52 tasks
KristofferC pushed a commit that referenced this pull request Mar 30, 2023
Let Base handle concatenation of arrays and numbers

(cherry picked from commit e6c84a1)
@KristofferC KristofferC removed the backport 1.9 Change should be backported to release-1.9 label Mar 31, 2023
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] backport 1.8 Change should be backported to release-1.8 linear algebra Linear algebra
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Poor performance of vcat/hcat/hvcat on mixed input types
5 participants