Working implementation of blocked QR #17
Conversation
|
NOT READY FOR SUBMISSION. Lots of comments to improve and small things to clean up. Sorry for the large PR. It took a while to understand what was actually going on with the individual routines. |
|
Okay. Comments fixed, and tests fully added. PTAL. |
|
Don't review this yet. I have been working on some new functions and the tests will be restructured a bit. At that point we can find the best way to merge the changes. |
native/dgeqr2.go
Outdated
There was a problem hiding this comment.
Dgeqr will panic if work is too short.
|
Alright. Found some bugs. Full implementation of Dgels! Working on better documentation/testing now. |
|
Ok! Comments fixed and tests added. The tests will fail right now because of gonum/blas#130. PTAL I know this is a lot. Let me know if I should split it up. Adding the extra functionality allowed me to better understand what was going on and provide more cross tests. |
lapack.go
Outdated
There was a problem hiding this comment.
Don't need the parentheses on types (also below).
|
I'm not entirely sure where to start with this. Can you direct me? |
|
Start with Dgeqr2. This will lead you to Dlarf and Dlarfg. Once done, you can move to Dgeqrf, which does the same thing as Dgeqr2, but is the blocked version, which will lead you to Dlarfb and Dlarft. With those in place, Dgelq2 and Dgelqf are the same idea. Then go to Dormqr and Dormlq which are a small step after that. Then look at Dlange, which will seem like a bit of a non-sequitur. However, this puts all the major pieces in place to examine Dgels. This will lead you to a few more simple auxiliary routines . |
native/general.go
Outdated
There was a problem hiding this comment.
Done.
Do you anything about the history of this function? It's useful here, but it seems like a weird thing to have in the math package (and yet a bunch of languages seem to have it easily accessible). Is this a lot more useful than I think?
There was a problem hiding this comment.
I use it. I can't add anything beyond that.
|
It was pretty hard to concentrate through dlarfb, so if we could generate that it would be good - I can look again later if you want. Throughout, it should note that insufficiently long []float64 will panic. It would be good to have the new tests passing with the cgo code if possible. LGTM with trepidation after comments addressed. |
|
@vladimir-ch would you please also take a look. |
|
What do you mean "If we could generate that part it would be good"? |
|
I'm just wondering if there is some simpler way of representing the structure here (either in the code itself - probably not - or in some code generator) that reduces the difficulty of reviewing and maintaining it.
|
native/dlabad.go
Outdated
There was a problem hiding this comment.
What would you like me to say? Here is the original comment
DLABAD takes as input the values computed by DLAMCH for underflow and
overflow, and returns the square root of each of these values if the
log of LARGE is sufficiently large. This subroutine is intended to
identify machines with a large exponent range, such as the Crays, and
redefine the underflow and overflow limits to be the square roots of
the values computed by DLAMCH. This subroutine is needed because
DLAMCH does not compensate for poor arithmetic in the upper half of
the exponent range, as is found on a Cray.
There was a problem hiding this comment.
SGTM except that it doesn't return anything - do you mean record?
There was a problem hiding this comment.
That was the netlib comment for the original LAPACK routine. Since parameters are passed by reference, "return" really means "overwrite". This doesn't apply since Go guarantees IEEE-754. We could also delete it entirely.
There was a problem hiding this comment.
I'll leave it up to you as to whether it's kept, but it seems to me that if it is kept, either you accept pointers (something we avoid) or return the values unaltered.
There was a problem hiding this comment.
It was unclear to me how closely to follow the original code. I'm all for deleting code if we think that's okay.
native/dgeqr2.go
Outdated
|
Sorry for the flood of nitpicking. I will try to give it a second pass to look more at the code itself in the afternoon when I have recovered some strength to concentrate. |
native/dgelqf.go
Outdated
There was a problem hiding this comment.
This condition does not seem to be checked.
|
I think it LGTM after addressing my comments. Great work! |
|
Both of you deserved a round of applause for this behemoth. |
|
Thank you as well Dan. Sorry again this was so large. I'll keep them smaller in the future even if I need to implement a few more for understanding. |
Improved function documentation Fixed dlarfb and dlarft and added full tests Added dgelq2 Working Dgels Fix many comments and tests Many PR comment responses Responded to more PR comments Many PR comments
Working implementation of blocked QR, LQ and linear solve using QR/LQ
No description provided.