Skip to content

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.

# 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