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

bi/tri/symdiagonal matrix multiplication should return sparse matrices #28343

Closed
wants to merge 2 commits into from

Conversation

mcognetta
Copy link
Contributor

@mcognetta mcognetta commented Jul 29, 2018

PR #15505 added faster bi/tri/symdiagonal matrix multiplication methods, but the fallback caused them to output dense matrices which removed any speed-up:

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)

at bidiag.jl:507

This PR changes it so these multiplication methods return sparse arrays instead of dense.

Some basic timings:
Before:

julia> A = Tridiagonal(rand(5000,5000))
julia> B = Tridiagonal(rand(5000,5000))
julia> @time A*B
  3.957645 seconds (10 allocations: 572.205 MiB, 2.23% gc time)

After:

julia> A = Tridiagonal(rand(5000,5000))
julia> B = Tridiagonal(rand(5000,5000))
julia> @time A*B
  0.063493 seconds (39 allocations: 1.078 MiB)

I think there are a few more combinations of special matrices that can benefit from something like this.

Also, it may be worth discussing if we should just return band matrices directly instead of sparse.


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)

@mcognetta
Copy link
Contributor Author

Since this uses spzeros it had to be moved to SparseArrays.jl. Some other functions are here also due to #25249 also because they use spzeros. This should probably be fixed.

@mcognetta mcognetta changed the title bi/tri/symdiagonal matrix multiplication should output sparse matrices bi/tri/symdiagonal matrix multiplication should return sparse matrices Jul 29, 2018
@kshyatt kshyatt added the linear algebra Linear algebra label Aug 3, 2018
@kshyatt kshyatt requested a review from Sacha0 August 3, 2018 13:50
Copy link
Member

@Sacha0 Sacha0 left a comment

Choose a reason for hiding this comment

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

(Edit: Whoops, meant to comment instead of approve!)

Thanks @mcognetta! :)

Overall this change strikes me as reasonable. I would consider leaving the existing method in LinearAlgebra and overwriting it in SparseArrays; that way, at some future point when LinearAlgebra, SparseArrays, and friends are less coupled, these operations will still work when loading only LinearAlgebra. @andreasnoack and @mbauman may also have thoughts on the overall direction here.

Apart from the above, perhaps test this behavior? :)

@mcognetta
Copy link
Contributor Author

Thanks for the comments. Actually I am in the process of making a new PR that subsumes this one (and one of my others about a similar thing). It will have a few more optimizations and all the related tests.

If leaving it in LinearAlgebra and overwriting it in SparseArrays is the way to go I'll include that in my other PR. Thanks for that suggestion.

@ViralBShah
Copy link
Member

This has been resolved now, and the result is a sparse matrix and significantly faster.

@ViralBShah ViralBShah closed this Jul 6, 2019
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