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

fixed dense times diagonal matrix multiplication #28345

Closed
wants to merge 2 commits into from

Conversation

mcognetta
Copy link
Contributor

Fixing JuliaLang/LinearAlgebra.jl#525 to make dense times diagonal matrix multiplication return dense matrices.

Some basic timings:

Before:

julia> A = Tridiagonal(rand(2000,2000))
julia> B = rand(2000,2000)
julia> @time A*B
  0.075236 seconds (6 allocations: 30.518 MiB, 87.87% gc time)
julia> @time B*A
  4.515886 seconds (50 allocations: 66.017 MiB, 0.07% gc time)

After:

julia> A = Tridiagonal(rand(2000,2000))
julia> B = rand(2000,2000)
julia> @time A*B
  0.023720 seconds (6 allocations: 30.518 MiB, 7.70% gc time)
julia> @time B*A
  0.017339 seconds (6 allocations: 30.518 MiB, 10.38% gc time)

This also affects dense times Bidiagonal and SymTriDiagonal matrix multiplication.


julia> versioninfo()
Julia Version 0.7.0-beta2.0
Commit b145832402* (2018-07-13 19:54 UTC)
Platform Info:
  OS: Linux (x86_64-linux-gnu)
  CPU: Intel(R) Core(TM) i7-6600U CPU @ 2.60GHz
  WORD_SIZE: 64
  LIBM: libopenlibm
  LLVM: libLLVM-6.0.0 (ORCJIT, skylake)

Copy link
Member

@andreasnoack andreasnoack left a comment

Choose a reason for hiding this comment

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

The new definition triggers an ambiguity so you probably need to add a few more definitions or simply split the definition into the components of the SpecialMatrix union.

@@ -504,6 +504,7 @@ const SpecialMatrix = Union{Bidiagonal,SymTridiagonal,Tridiagonal}
# to avoid ambiguity warning, but shouldn't be necessary
*(A::AbstractTriangular, B::SpecialMatrix) = Array(A) * Array(B)
*(A::SpecialMatrix, B::SpecialMatrix) = Array(A) * Array(B)
*(A::AbstractMatrix, B::SpecialMatrix) = A_mul_B_td!(zeros(eltype(A),size(A)...), A, B)
Copy link
Member

Choose a reason for hiding this comment

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

Not sure this is right. Should probably be similar(A, T) where T is computed with matprod. See matmul.jl.

@kshyatt kshyatt added the linear algebra Linear algebra label Aug 3, 2018
@ViralBShah
Copy link
Member

Bump. Would be nice to get this in. Performance of the reported case is still poor.

@mcognetta
Copy link
Contributor Author

@ViralBShah Thanks for bringing this back to my attention. I am going to close this in favor of a new PR since this is so far behind and the changes modify some things unrelated to the title.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
linear algebra Linear algebra
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants