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.

Sign up for GitHub

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

Jump to bottom

RFC: Use sparse triangular solvers for sparse triangular solves. #13793

Merged
merged 2 commits into from
Nov 2, 2015
Merged

RFC: Use sparse triangular solvers for sparse triangular solves. #13793

merged 2 commits into from
Nov 2, 2015

Conversation

andreasnoack
Copy link
Member

@andreasnoack andreasnoack changed the title Use sparse triangular solvers for sparse triangular solves. WIP: Use sparse triangular solvers for sparse triangular solves. Oct 27, 2015
@andreasnoack
Copy link
Member Author

These require test coverage and I've also just realized that fwd/bwdTriSolve! don't work with the Triangular views. They'd have to be modified.

@kshyatt kshyatt added linear algebra Linear algebra sparse Sparse arrays labels Oct 27, 2015
@andreasnoack andreasnoack changed the title WIP: Use sparse triangular solvers for sparse triangular solves. RFC: Use sparse triangular solvers for sparse triangular solves. Oct 27, 2015
mschauer
mschauer reviewed Oct 28, 2015
View reviewed changes
base/sparse/linalg.jl
end
ncol = chksquare(A)
nrowB, ncolB = size(B, 1), size(B, 2)
ncol = LinAlg.chksquare(A)
if nrowB != ncol
throw(DimensionMismatch("A is $(ncol)X$(ncol) and B has length $(n)"))
Copy link
Contributor

Choose a reason for hiding this comment

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

"has $nrowB rows"

@andreasnoack
Use sparse triangular solvers for sparse triangular solves. Fixes #13…
81753cc 
…792.

Make fwd/bwdTriSolve! work for triagular views

Add check for triangular matrices in sparse factorize
@andreasnoack
Copy link
Member Author

This required a little more work than expected because I hit the limit for union sizes and therefore the return type for \ began to be inferred as Any for sparse matrices.

@tkelman I've split the pr into two commits. The first one is a candidate for backporting because I think it was a bug that \ for Lower/UpperTriagular{SparseMatrixCSC} used the generic (dense) fallback. The second commit is some simplifactions of all the \ dispatch and I don't think that is a candidate for backporting.

tkelman
tkelman reviewed Nov 1, 2015
View reviewed changes
base/sparse/umfpack.jl
end

function show(io::IO, F::UmfpackLU)
println(io, "UMFPACK LU Factorization of a $size(F) sparse matrix")
Copy link
Contributor

Choose a reason for hiding this comment

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

need another paren around $(size(F))

@andreasnoack
Consolidate the \ methods for sparse and dense and make the necessary…
87e26c5 
… adjustment

to other methods.

Solve real lhs problems with complex rhs by reinterpreting the rhs.

Let factorize(SparseMatrixCSC) check for tringular matrices
andreasnoack added a commit that referenced this pull request Nov 2, 2015
@andreasnoack
Merge pull request #13793 from JuliaLang/anj/sparsesolve
af3e15d 
RFC: Use sparse triangular solvers for sparse triangular solves.
@andreasnoack andreasnoack merged commit af3e15d into master Nov 2, 2015
@andreasnoack andreasnoack deleted the anj/sparsesolve branch November 2, 2015 19:08
andreasnoack added a commit that referenced this pull request Nov 3, 2015
@andreasnoack @tkelman
Use sparse triangular solvers for sparse triangular solves. Fixes #13…
6fa0f6e 
…792.

Make fwd/bwdTriSolve! work for triagular views

Add check for triangular matrices in sparse factorize

(cherry picked from commit 81753cc)
ref #13793
tkelman
tkelman reviewed Jan 25, 2016
View reviewed changes
base/sparse/linalg.jl
@@ -800,6 +826,15 @@ scale{T,Tv,Ti}(b::Vector{T}, A::SparseMatrixCSC{Tv,Ti}) =
function factorize(A::SparseMatrixCSC)
m, n = size(A)
if m == n
if istril(A)
if istriu(A)
return return Diagonal(A)
Copy link
Contributor

Choose a reason for hiding this comment

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

@ScottPJones pointed out this typo on the mailing list. Apparently this isn't an error?

Copy link
Member Author

Choose a reason for hiding this comment

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

No. I also thought it would be an error, but then realized that the line actually has test coverage. However, a few other lines in factorize(SparseMatrixCSC) don't so I've prepared a small commit with extra tests which also removes the extra return.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
linear algebra Linear algebra sparse Sparse arrays
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

A \ b slow for triangular sparse matrix.
4 participants
@andreasnoack @mschauer @tkelman @kshyatt