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

Improve performance of svd and eigen of Diagonals #43856

Merged
merged 13 commits into from
Feb 8, 2022
Merged

Conversation

dkarrasch
Copy link
Member

This reduces lots of intermediate array allocations, and now even works with (Furlong) units. I'll add a test for that case later.

This reduces lots of intermediate array allocations, and now even works with (Furlong) units.
@dkarrasch dkarrasch added linear algebra Linear algebra performance Must go faster labels Jan 18, 2022
@dkarrasch
Copy link
Member Author

This is a wonderful example why splitting LinearAlgebra and SparseArrays was really useful. See the benchmarks:

julia> D = Diagonal(randn(1024));

julia> @btime svd($D);
  16.898 ms (10275 allocations: 866.75 KiB) # old behavior with sparse U and Vt

julia> @btime svd($D);
  11.371 ms (2076 allocations: 40.38 MiB) # before this PR: dense output + temporary allocations

julia> @btime svd($D);
  1.506 ms (8 allocations: 16.02 MiB) # this PR: dense output + seemingly no unnecessary allocations

While sparse output is good from a memory perspective, writing into an initially empty sparse matrix is slow. I think the same is true for the multiplications listed in 14154fc#commitcomment-64103782. Honestly, I hadn't seen that coming. 😅 So, I believe the "new" defaults (similar(::StructuredMatrix, T, size) = zeros() instead of spzeros) are really not bad, plus the old behavior is easy to recover.

Pinging some involved people to make sure they join me in my little github celebration 🎉 : @vtjnash @KristofferC @ViralBShah 😄

@dkarrasch dkarrasch changed the title Optimize performance of svd(::Diagonal) Improve performance of svd and eigen of Diagonals Jan 20, 2022
@dkarrasch
Copy link
Member Author

dkarrasch commented Jan 20, 2022

This also fixes an issue in sorted eigen of Diagonals, where the sort! was acting on D.diag directly. I'll prepare a minimal fix suitable for backporting.

@dkarrasch dkarrasch added backport 1.6 Change should be backported to release-1.6 backport 1.7 labels Jan 21, 2022
@dkarrasch dkarrasch closed this Feb 7, 2022
@dkarrasch dkarrasch reopened this Feb 7, 2022
@dkarrasch dkarrasch merged commit 6bad936 into master Feb 8, 2022
@dkarrasch dkarrasch deleted the dk/svddiag branch February 8, 2022 09:17
@KristofferC KristofferC mentioned this pull request Feb 15, 2022
40 tasks
antoine-levitt pushed a commit to antoine-levitt/julia that referenced this pull request Feb 17, 2022
@KristofferC KristofferC mentioned this pull request Feb 19, 2022
50 tasks
LilithHafner pushed a commit to LilithHafner/julia that referenced this pull request Feb 22, 2022
LilithHafner pushed a commit to LilithHafner/julia that referenced this pull request Mar 8, 2022
KristofferC pushed a commit that referenced this pull request Mar 15, 2022
KristofferC pushed a commit that referenced this pull request Mar 15, 2022
@KristofferC
Copy link
Member

I tried to backport this to 1.6 but got a test error:

julia> E = eigen(Du)
Eigen{Furlong{0, Float64}, Furlong{1, Float64}, Matrix{Furlong{0, Float64}}, Vector{Furlong{1, Float64}}}
values:
3-element Vector{Furlong{1, Float64}}:
  Furlong{1, Float64}(-0.5153917252601611)
 Furlong{1, Float64}(-0.23809233488008882)
   Furlong{1, Float64}(0.1571713328338422)
vectors:
3×3 Matrix{Furlong{0, Float64}}:
 Furlong{0, Float64}(1.0)  Furlong{0, Float64}(0.0)  Furlong{0, Float64}(0.0)
 Furlong{0, Float64}(0.0)  Furlong{0, Float64}(1.0)  Furlong{0, Float64}(0.0)
 Furlong{0, Float64}(0.0)  Furlong{0, Float64}(0.0)  Furlong{0, Float64}(1.0)

julia> @test Matrix(E) == Du
Error During Test at REPL[55]:1
  Test threw exception
  Expression: Matrix(E) == Du
  AssertionError: p == q
  Stacktrace:
    [1] Furlong{1, Float64}(x::Furlong{0, Float64})
      @ Main.Furlongs ~/julia/test/testhelpers/Furlongs.jl:20
    [2] convert(#unused#::Type{Furlong{1, Float64}}, x::Furlong{0, Float64})
...

at

@test map(x -> x.val, Matrix(F)) map(x -> x.val, Du)

@dkarrasch
Copy link
Member Author

I can reproduce. I think the diagonal multiplication code must have changed between versions, and on v1.6 it cannot multiply vecs::Matrix{Furlong{0}} * Diagonal(vals::Vector{Furlong{1}}. It tries to copy vecs into a matrix of the result type, which impossible. Long story short, it fails for reasons outside of this commit, so can we still backport but omit the unitful tests?

@oscardssmith
Copy link
Member

why are we back porting? isn't this just a performance improvement?

@KristofferC
Copy link
Member

and now even works with (Furlong) units

So it is unit correct now.

@dkarrasch
Copy link
Member Author

First of all, it contains a fix for the aliasing of D.diag in sorted eigendecompositions.

@KristofferC KristofferC removed the backport 1.6 Change should be backported to release-1.6 label May 16, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
linear algebra Linear algebra performance Must go faster
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants