SPQR: allow user-selected ordering #35599
Conversation
stdlib/SuiteSparse/src/spqr.jl
Outdated
| # Apply right permutation and extract solution from X | ||
| return getindex(X, ntuple(i -> i == 1 ? invperm(F.cpiv) : :, Val(ndims(B)))...) | ||
| # NB: cpiv == [] if SPQR was called with ORDERING_FIXED | ||
| return length(F.cpiv) == 0 ? getindex(X, ntuple(i -> i == 1 ? (1:size(F,2)) : :, Val(ndims(B)))...) : getindex(X, ntuple(i -> i == 1 ? invperm(F.cpiv) : :, Val(ndims(B)))...) |
There was a problem hiding this comment.
Style nit: please keep line length reasonable. So usually about 1 anonymous function per expression.
There was a problem hiding this comment.
I had trouble with this when I tested. That's what I try to explain above. Adding a return statement for the special case causes threaded tests to fail. I'm not sure why. I'll try again.
There was a problem hiding this comment.
Oh well, nevermind. I cleaned it up and it seems ok.
|
Specifying the ordering as a value from an array is a bit difficult to use. Perhaps use |
|
You don't refer to them as a value in an array. You refer to them by name: |
|
Ah thanks. I didn't notice that was the case only in |
|
Sorry I forgot to update docs and news. Will do in a separate PR. |
|
Could you indicate where I should modify the docs? Every time I read https://docs.julialang.org/en/v1/stdlib/LinearAlgebra/#LinearAlgebra.qr I'm surprised that it doesn't seem to talk about sparse QR at all, but I can't find another place that does. The sparse QR doesn't take a |
|
Yes, unfortunately none of the sparse factorizations have documentation. I think the right place to put them is in the sparse matrix implementation, and the doc system should find it. At least that's my understanding. @fredrikekre should be able to confirm that. |
|
All the sparse factorizations have docstrings but it does look like that they aren't included in the documentation. I guess we need to generate a file similar to https://github.com/JuliaLang/julia/blob/master/stdlib/LinearAlgebra/docs/src/index.md in the |
|
@andreasnoack Can you open a separate issue for that? |
5 participants
This PR adds the
orderingkwarg toqr()for sparse matrices (which calls SuiteSparseQR) so the user can select one of the orderings exposed by SPQR.The default value of
orderingisORDERING_DEFAULTso that the default behavior is unchanged.A little care must be taken when returning from
_ldiv_basicbecause ifordering == ORDERING_FIXED, SPQR returns an empty column permutation (to indicate that no permutation occurred).I tried writing thereturnstatement so it's more readable but got errors in the threaded SuiteSparse tests.I added tests for over- and under-determined problems.
@andreasnoack