Adddgesvd #79
Adddgesvd #79
Conversation
|
Woo! This provides a partial implementation of Dgesvd. It only supports the case where the full U and V are computed. We had previously thought we may only support computing U and V (no option to not). I'm not entirely sure anymore this is a good idea, as this is a huge amount of memory overhead if m >> n. A full implementation of the function is ~ 10x more work. We could possibly not support overwriting the initial A. This would save a bunch of work as this is the confusing case. The strides are hard to parse because of row/column major differences. @kortschak Do you know what the "superb" field means? Is it possible to have a specific SVDJob type, or should I switch those types? I fixed a bug in Dorgbr and edited the tests, but I'm not convinced the current implementation is correct. It seems like Q should have to be m x n and not m x k, but the SVD tests fail if I change that logic. I don't understand from the comment why Q should be m x k. If someone could take a second glance it would be appreciated. |
Where? |
|
Superb field is the last entry in the clapack code. I have work there, but I'm not sure that's what that means (no other function so far takes in temporary memory). The svdjob type is an individual one I created here, the autogenerated code takes in an unnamed job |
|
I'm almost done with the svd implementation ignoring overwrite. I then need to figure out how to write tests. No point in reviewing until that's correct (aside from cgo questions) |
|
It's a place to retain the 1 to min(m-2,n-2)th elements of work. Seems sort of odd.
https://github.com/xianyi/OpenBLAS/blob/develop/lapack-netlib/LAPACKE/src/lapacke_dgesvd.c
|
|
Okay, found a few bugs that make things make more sense. Getting close to a working implementation. |
|
It almost works now. It works if dgebd2 is called instead of the loop in dgebrd. This implies there is something wrong with the Dgebrd loop, but I don't see it. |
|
Found it finally |
|
Okay! I think this works now. I don't know how to test the "insufficient work" branches in an automated manner as the recommended work is bigger than just one greater. I did test them by hand and they work. PTAL! |
cgo/lapack.go
Outdated
There was a problem hiding this comment.
Here and on the next three lines use spaces for the alignment of the second column.
|
I'll try to take a look at the code itself later. |
|
Done. Thanks for the comments. |
cgo/lapack.go
Outdated
native/dgesvd.go
Outdated
|
LGTM after comments addressed, but wait for @vladimir-ch. |
|
Addressed PR comments, minus the outstanding issue about what to do with minwork. PTAL @vladimir-ch |
cgo/lapack.go
Outdated
There was a problem hiding this comment.
s/M columns/m columns/
Also below
|
What a joy every time I encountered |
|
That was my thought too! If you want to figure out the correct mapping of the strides, I could be convinced to cede that job |
# The first commit's message is: fix merge # The 2nd commit message will be skipped: # Add Dgesvd and test # The 3rd commit message will be skipped: # implemented work thing
No description provided.