Enh/image basis perf #46
Conversation
it seems that droptol! after each reduce is much faster for sparse matrices
Codecov Report
@@ Coverage Diff @@
## master #46 +/- ##
==========================================
- Coverage 85.75% 85.55% -0.21%
==========================================
Files 16 16
Lines 1053 1066 +13
==========================================
+ Hits 903 912 +9
- Misses 150 154 +4
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
| F = qr(M) | ||
| M_rank= rank(F) | ||
| result = let tmp = F.Q * Matrix(I, size(F.Q, 2), M_rank) | ||
| sparse(tmp[invperm(F.prow), :]') |
There was a problem hiding this comment.
This is ok when returning Upis as the type will be SparseMatrixCSC with M_rank rows. However, when using this routine inside invariant_vectors(), e.g. when computing invariant_vectors for basis_full without any specialized method for ByPermutations or BySignedPermutations, we may need to convert result to vector of sparsevec.
There was a problem hiding this comment.
Also, as discussed would be nice to make the output type of invariant_vectors same as Upis since it can be regarded as Upis for the trivial subrep, that is SparseMatricSCS. This also includes making the type T in WedderburnDecomposition propagate to the results of invariant_vectors so that SparseMatricSCS{T} is returned (both for Upis and for invariant_vectors). @kalmarek
There was a problem hiding this comment.
@thinh-le yeah, I'm convinced that You're suggestion is a good solution, but let change the format of invariant vectors in a separate pull
There was a problem hiding this comment.
@kalmarek Thanks. I also believe that users would rarely use invariant vectors individually (if really wanted, views provide nice solution) but as matrix-vector multiplication (which is fast for both sparse and dense BLAS).
Also, I had a look at this issue #39 and for me it does not matter much if the user receive a row or column based Upis:
- adjoint is lazy
- matrix multiplication of Upi plays nice with adjoint (and fast when everything is sparse)
Maybe we should pick one that is fast for internals..
@thinh-le have a look if you have any comments on
image_basis!for sparse matrices; note that this method (unlike the others) relies on the fact thatAis square and symmetric which I'm not enthusiastic about ;)