Add Dgetf2 (unblocked LU algorithm) and tests #22
Conversation
|
PTAL @kortschak |
native/dgetf2.go
Outdated
There was a problem hiding this comment.
I don't know if we do this elsewhere, but we probably should if not. "a" here should be "A" since we are not actually describing the abstract matrix in the parameter a as we do in mat64; m, n, a and lda are required to do this. So, leave the parameter as "a" and change the doc to "A" where appropriate. Probably we need to vet all the docs at some point to ensure we are doing this consistently.
|
Sorry, I forgot I cheated on the swapping. Fixing now. |
|
Add tests for the cgo implementation. |
|
mat64 will use the blocked version of the algorithm (dgetrf). Do we want cgo tests even though mat64 won't import dgetf2? |
|
Yes, please. |
native/dgetf2.go
Outdated
There was a problem hiding this comment.
Assuming you mean -1, yes. Nice catch.
There was a problem hiding this comment.
The loop in fortran is 1,m-j which is 1 <= i <= m-j; shifted to 0-based 0 <= i <= m-j-1 which is 0 <= i < m-j. So no adjustment at all. Am I missing something?
There was a problem hiding this comment.
Yes. j is 1-indexed, so to shift from Fortran j to Go j you also need to subtract 1.
There was a problem hiding this comment.
For example, for the first column, in fortran it's 1 <= i <= m -1.
|
The issue of blas/native v blas/cgo and their interaction with lapack/native v lapack/cgo I think needs to be sorted out. Proviously the configuration was simple since we didn't use lapack, now there are two knobs where one is ignored in some states of the other. This needs at least to be documented in mat64. |
|
Added cgo tests. Agreed on the need for mat64 documentation. |
There was a problem hiding this comment.
add the name ok to the return - at the moment the documentation make it look like you are returning (isSingular bool).
There was a problem hiding this comment.
I think it is returning isSingular. The factorization doesn't stop when a singularity is found.
There was a problem hiding this comment.
In the OpenBLAS code info is set to != 0 when it's singular and the fortran specifies info == 0 indicates OK. Here you set ok = false when you see a singularity. And you call it "ok".
There was a problem hiding this comment.
Oh, I see your point. Yes.
|
I'm not sure why the cgo code is failing on <go1.5. The OpenBLAS code is here and it looks like it should be doing the right things wrt the return boolean. |
|
What's more, it passes on my machine. |
|
Mine too. |
|
Would you make an exploratory change to clapack.Dgetf2 to print the return value before calling isZero so we can see what is happening on the travis machine? Also add |
|
I'll make the change in a few minutes. |
|
I suggested wrong line in install.sh - I've pushed the correct version now. |
|
So I shouldn't add the cgo test lines? Sorry, I'm also helping a new gopher at the moment. |
|
Yes, add them, and the other things (see #23 but wait for it to succeed). |
|
Pushed. Here's what it prints on my machine |
|
So the last two give different results. Unfortunately the rev-parse is on the wrong line (note where it is on #23 - I was wrong in my original instructions), so we can't see which OpenBLAS is being used. |
|
|
|
OK, I'm officially bewildered. I've built OpenMathLib/OpenBLAS@3f1b576 which is the sha that was reported in the #23 tests and so presumably is what was built here also, and then tested that on my machine. I get pass and the same output as you. |
|
@xianyi Do you have any ideas why OpenBLAS Dgetf2 might be returning singular matrices as not singular, but only on travis hardware? |
|
I'm not sure if it is related, but in optimize we also had problems with tests failing only on Travis, they were running differently. As for the reason, I don't know. |
|
I assume I should revert the cgo changes, or are they still useful? |
|
Don't revert them, reset --hard e86d740 and push -f. |
btracey
force-pushed
the
adddgetf2
branch
2 times, most recently
from
e86d740 to
abd7e2b
Compare
cgo/lapack.go
Outdated
|
Oh, oops. |
fixed incorrect comment and incorrect test Real permutations and PR comments Add cgo tests fix indexing error change return to ok fix okays fix ok
|
What is the status of this PR? Are we waiting until we have a resolution on the OpenBLAS error? |
|
Press ahead, neither of us can reproduce the failure, one at least with the same ref for OpenBLAS. I guess we can open an issue with xianyi/OpenBLAS; it might go better if we can write a C reproducer to have travis build - this should be relatively straight forward.
|
Add Dgetf2 (unblocked LU algorithm) and tests
No description provided.