faster implementation of rank(::QRPivoted) fixes #1128 #1129
Conversation
|
@stevengj made the changes you suggested. Thanks! |
|
resolved all the suggestions |
|
I had a general question: is it okay to squash commits and (force) push to my branch? That will reduce the number of commits in this PR and remove all references to the SVD.jl files that I accidentally included (and removed in a later commit) ? |
|
Yes, it's fine, especially if it makes the PR easier to review |
|
It doesn't hurt to squash, but it shouldn't be necessary — if we use the "squash and merge" button, then the PR will be squashed before merging anyway. |
|
Thanks! @stevengj so it should be fine if there are multiple commits? |
|
Based on a suggestion in another PR, I simplified the |
|
also changed |
|
I would encourage including a minimal and self-contained description in the commit message, so that it's easier to follow the git log. |
|
What else needs to be done to merge this PR? |
I tried to be descriptive but some of the changes were truly minor. I'll do better in the future. It's also one reason I wanted to squash commits, but since @stevengj mentioned that we can just "squash and merge" the PR I didn't do it. |
|
Sorry I meant in the PR description, as that will become the commit description when the PR is squashed and merged. Currently it just says that this fixes an issue, but a bit more detail would help in understanding exactly what the PR contributes. |
|
Other than the comment on the PR description, this looks good to me |
|
@jishnub thanks! I updated the PR description |
|
Is this method being tested already? Otherwise we might want to add a test |
|
It is being tested here: Line 531 in 8ab7e09 do we need/want to change the test set name? |
|
Ok, no this is fine. I think we can go ahead and merge this. |
|
Thanks for the PR! |
| end | ||
|
|
||
| function rank(A::QRPivoted; atol::Real=0, rtol::Real=min(size(A)...) * eps(real(float(one(eltype(A.Q))))) * iszero(atol)) | ||
| function rank(A::QRPivoted; atol::Real=0, rtol::Real=min(size(A)...) * eps(real(float(eltype(A)))) * iszero(atol)) |
There was a problem hiding this comment.
Note that one(eltype(A)) would be better here, in principle, in order to handle the case of dimensionful matrices — the rtol should always be dimensionless, and one strips any units.
(I'm not sure if we support dimensionful matrices yet in QRPivoted — I'm guessing we don't — but it's nice to think about the possibility of unitful numbers when implementing algorithms.)
There was a problem hiding this comment.
Sorry, I made that change based on a comment in #1127 . Before making the change I compared eps(eltype(A)) and eps(one(eltype(A))) and verified they give the same answer. I can submit another PR to put back the one in there. Also I dont think I understand what you mean by dimensionful here. Can you point me to a resource to read?
3 participants
(fixes #1128) the current implementation of
rank(::QRPivoted)is slow and inefficient because of repeated calls toA.Reach of which triggers memory allocation. this PR fixes that by usingA.factorsinstead which does not cause memory allocations. The proposed implementation is orders of magnitude faster and avoids unnecessary memory allocations altogether.