Native svd #16205
Native svd #16205
Conversation
rahulghangas
commented
Aug 6, 2020
•
rahulghangas
commented
- Add tests
…e to all 2-D matrices
There was a problem hiding this comment.
@rahulghangas - Nice start. I took a high-level pass over this and have some comments. I will pass the review torch to @dgarvit to take the next more in-depth review iteration.
Some notes:
- Performance
- I am seeing a lot of nested parallelism and parallel forall loops over small ranges.
- I would experiment to see if the nested parallel loops are beneficial or are degrading performance due to task creation overhead / too many tasks running in parallel
- Docs/comments
- I think we should reference the numerical recipes book chapter in the comment block above the function. It's good to cite references, and also helpful for someone else trying to follow the code.
- We can update the other
svd()comment block to remove the note about requiring LAPACK for svd- Or maybe the native
svd()deserves its own separate documented function (what do others think?)
- Or maybe the native
- Speaking of comments, this code could use some more comments describing what's going on in general 😄
- Testing
- We could use more testing (correctness and performance). More details inline.
BTW - there are some interesting improvements discussed in #16200 (comment) (I think you're following along). We should discuss these as follow-on improvements (as time permits this summer).
Thanks!
| @@ -0,0 +1,26 @@ | |||
| use LinearAlgebra; | |||
There was a problem hiding this comment.
Good initial tests. I think it would be beneficial to cover some of the edge cases (over/under-determined matrices, singular matrices).
The scipy.linalg has some example cases to check against.
If we end up not adding complex support for this PR, we ought to add a future for it (let me know if you have any questions about that process).
There was a problem hiding this comment.
Also, a performance test would be great, since we intend to optimize this implementation going forward.
In fact, getting LAPACK svd vs. Chapel svd on the same nightly performance-tracking plot could be really cool
There was a problem hiding this comment.
I'll add those ASAP
|
|
||
| if i < m { | ||
|
|
||
| forall k in i..<m with (+ reduce scale) do scale += abs(U[k,i]); |
There was a problem hiding this comment.
Can this be written as something like this? (note: I didn't try this myself)
| forall k in i..<m with (+ reduce scale) do scale += abs(U[k,i]); | |
| scale = + reduce (abs(U[1..<m, i])); |
There was a problem hiding this comment.
This applies to a bunch of reductions below as well.
There was a problem hiding this comment.
I did that because Damian mentioned something about it in the mailing lists - explicit forall loops performing better for reductions than array slicing. I'll double check the emails and perform some local testing as well to confirm
| return if b >= 0:t then (if a >= 0 then a else -a) else (if a >= 0 then -a else a); | ||
| } | ||
|
|
||
| private inline proc pythag(a : ?t, b : t) : t { |
There was a problem hiding this comment.
Even private helper functions deserve doc strings IMO.
| @@ -1757,6 +1757,261 @@ proc svd(A: [?Adom] ?t) throws | |||
| return (u, s, vt); | |||
| } | |||
|
|
|||
| pragma "no doc" | |||
| proc svd(A : [?Adom] ?eltType, iterations : int = 30, eps : real = 1e-8) | |||
There was a problem hiding this comment.
These news args will need to be documented. Maybe we want 2 separate documented functions for svd since their interface doesn't 100% match.
There was a problem hiding this comment.
I would be in favor of having separate documentations until both interfaces can be unified
|
Hi @rahulghangas -- will you be able to make some progress on the open discussions on this PR soon? If not, I'd encourage closing the PR and reopen once you have the time to work on it. |
|
Based on silence, I am closing this PR. @rahulghangas -- Please feel free to reopen when you have the time to work on this. |
