Add Dormbr and test #74

btracey · 2015-12-08T07:14:25Z

No description provided.

kortschak · 2015-12-08T23:30:43Z

Test for cgo? clapack has Dormbr.

btracey · 2015-12-08T23:40:17Z

We're back at that level? Exiting!

I just ran the cgo tests to make sure they pass before implementing the Dormbr tests. I hit the Dlantr panic. Not reproducable, but copying here for future reference

brendan:~/Documents/mygo/src/github.com/gonum/lapack/cgo$ go test
fatal error: unexpected signal during runtime execution
[signal 0xa code=0x2 addr=0xc820500000 pc=0x5447870]

runtime stack:
runtime.throw(0x425fda0, 0x2a)
    /Users/brendan/gover/go/src/runtime/panic.go:527 +0x90
runtime.sigpanic()
    /Users/brendan/gover/go/src/runtime/sigpanic_unix.go:12 +0x5a

goroutine 7 [syscall, locked to thread]:
runtime.cgocall(0x417ccd0, 0xc82003fa78, 0xc800000000)
    /Users/brendan/gover/go/src/runtime/cgocall.go:120 +0x11b fp=0xc82003fa30 sp=0xc82003fa00
github.com/gonum/lapack/cgo/clapack._Cfunc_LAPACKE_dlantr(0x4e554900000065, 0xa00000005, 0xc8204ffdc0, 0xb, 0x0)
    github.com/gonum/lapack/cgo/clapack/_obj/_cgo_gotypes.go:6622 +0x3a fp=0xc82003fa78 sp=0xc82003fa30
github.com/gonum/lapack/cgo/clapack.Dlantr(0x49, 0x55, 0x4e, 0x5, 0xa, 0xc8204ffdc0, 0x37, 0x37, 0xb, 0x80)
    /Users/brendan/Documents/mygo/src/github.com/gonum/lapack/cgo/clapack/clapack.go:3120 +0xa1 fp=0xc82003fab8 sp=0xc82003fa78
github.com/gonum/lapack/cgo.Implementation.Dlantr(0x585d449, 0x79, 0x83, 0x5, 0xa, 0xc8204ffdc0, 0x37, 0x37, 0xb, 0xc8204f8e80, ...)
    /Users/brendan/Documents/mygo/src/github.com/gonum/lapack/cgo/lapack.go:152 +0x21d fp=0xc82003fb20 sp=0xc82003fab8
github.com/gonum/lapack/cgo.(*Implementation).Dlantr(0x4327df0, 0x49, 0x79, 0x83, 0x5, 0xa, 0xc8204ffdc0, 0x37, 0x37, 0xb, ...)
    <autogenerated>:4 +0x12c fp=0xc82003fb90 sp=0xc82003fb20
github.com/gonum/lapack/testlapack.DlantrTest(0xc820094120, 0x585d6e0, 0x4327df0)
    /Users/brendan/Documents/mygo/src/github.com/gonum/lapack/testlapack/dlantr.go:73 +0x607 fp=0xc82003ff28 sp=0xc82003fb90
github.com/gonum/lapack/cgo.TestDlantr(0xc820094120)
    /Users/brendan/Documents/mygo/src/github.com/gonum/lapack/cgo/lapack_test.go:25 +0x76 fp=0xc82003ff68 sp=0xc82003ff28
testing.tRunner(0xc820094120, 0x4326850)
    /Users/brendan/gover/go/src/testing/testing.go:456 +0x98 fp=0xc82003ffa0 sp=0xc82003ff68
runtime.goexit()
    /Users/brendan/gover/go/src/runtime/asm_amd64.s:1721 +0x1 fp=0xc82003ffa8 sp=0xc82003ffa0
created by testing.RunTests
    /Users/brendan/gover/go/src/testing/testing.go:561 +0x86d

goroutine 1 [chan receive]:
testing.RunTests(0x4278158, 0x4326820, 0x16, 0x16, 0x1)
    /Users/brendan/gover/go/src/testing/testing.go:562 +0x8ad
testing.(*M).Run(0xc82003df08, 0x4006978)
    /Users/brendan/gover/go/src/testing/testing.go:494 +0x70
main.main()
    github.com/gonum/lapack/cgo/_test/_testmain.go:96 +0x116

goroutine 17 [syscall, locked to thread]:
runtime.goexit()
    /Users/brendan/gover/go/src/runtime/asm_amd64.s:1721 +0x1
exit status 2
FAIL    github.com/gonum/lapack/cgo 0.044s

kortschak · 2015-12-08T23:41:20Z

native/dormbr.go

I don't understand this for two reasons. First, blas.Side and blas.Transpose are not letters (we translate them in the wrappers around clapack in cgo for example), and second, opts is ignored by Ilaenv in native (in the FORTRAN it is only considered when ispec > 11 which the Go code panics for).

You're right that this is definitely wrong. What would you like me to do? I'm under the impression that Ilaenv is hypothetically tunable to the architecture, like ATLAS does, and I imagine this is the reason the values are passed explicitly instead of ignored even though the netlib implementation doesn't use them. Would you like me to construct and pass the string explicitly, or pass in empty?

We face the user with blas types rather than FORTRAN chars, so I'm less fussed by the translation thing (there is no internal native package like clapack - though the rest of Ilaenv does use letters), but it seems odd to me that ILAENV is called with ISPEC=1 and the OPTS are passed through with work to construct them.

The approach that you are taking is probably the only reasonable way to do it (alternative is as []int) since the types we use for these options are distinct. Maybe just add a comment that the current implementation does not use opts, but a future change may make that happen. Whether they get converted to 'L'/'R' and 'T'/'N' is up to you - and changeable later since nothing is done with them.

btracey · 2015-12-09T00:14:10Z

The cgo tests fail on Dormbr. This is weird, because before seeing this I would have put a high probability on the go code being correct. We know P and Q are computed appropriately. This is tested by Dgebd2, which both native and cgo pass. The only step after this is to multiply C by the respective matrix. We know Dgemm is correct (otherwise everything would break). It also seems unlikely I would have a bug in the native implementation (a translation from Fortran), and the test (independently coded and using different matrix data) that exactly cancel out to create a passing native test.

I pushed the cgo tests because I'm not sure I have time to debug at the moment and maybe you'll see something obvious.

kortschak · 2015-12-09T00:14:35Z

testlapack/general.go

Construct V and U.

btracey · 2015-12-09T00:24:08Z

Well, it's just a small subset that is failing (applyP for specific sizes), so it's probably a bug in the implementation and test.

kortschak · 2015-12-09T00:26:46Z

In a few days I can have a look using a direct FORTRAN call if you don't get to it first.

btracey · 2015-12-09T00:36:00Z

I wonder. The section that is failing has a call to Dormlq, and we have a comment in the cgo test for Dormlq

// Test disabled because of bug in c interface. Leaving stub for easy reproducer.

I'll try again with the latest OpenBLAS version tonight

vladimir-ch · 2015-12-09T02:00:15Z

cgo/lapack.go

off-diagonal

are stored? Or delete?

btracey · 2015-12-09T04:22:10Z

Symptom: Cgo only fails Dorml2 the test when Side == Right, and only under certain conditions.

I think I see the problem. From the documentation of Dorml2

// If side == blas.Left, a is a matrix of side k×m, and if side == blas.Right
// a is of size k×n.

From dormql_work.c

r = LAPACKE_lsame( side, 'l' ) ? m : n;
// later
LAPACKE_dge_trans( matrix_order, r, k, a, lda, a_t, lda_t );

This seems wrong. The size should depend on the value of size. Agreed? This matches that it only fails when side is right (since it's the correct size when left). Not fully clear why it passes under some of the Right conditions, but that may be due to an oddity of the way our test is constructed.

btracey · 2015-12-09T04:24:21Z

I see, I'm probably back to rediscovering OpenMathLib/OpenBLAS#615

I guess I'll go back to seeing if I can fix linking errors.

kortschak · 2015-12-09T04:25:14Z

I think you are right.

btracey · 2015-12-09T04:36:45Z

Nope, I get linking errors on tip as well.

kortschak · 2015-12-09T04:40:00Z

Working for me.

btracey · 2015-12-09T04:52:14Z

It compiles for me if I delete the following functions

cgeqpf
cggsvd
cggsvp
dgeqpf
dggsvd
dggsvp
sgeqpf
sggsvd
sggsvp
zgeqpf
zggsvd
zggsvp

btracey · 2015-12-09T04:58:25Z

The Dormlq test is still failing for me, even with the updated OpenBLAS. The transpode code has changed, but it now says (line 86 of dormlq_work.c)

LAPACKE_dge_trans( matrix_layout, k, m, a, lda, a_t, lda_t );

I think this is still wrong. The size of A depends on the side, as quoted above.

kortschak · 2015-12-09T04:59:12Z

Yes, I agree to both of those. BTW what OpenBLAS sha are you on?

btracey · 2015-12-09T05:00:44Z

brendan:~/software/OpenBLAS$ git rev-parse HEAD
25116788ef73cd8c5a0f7395818e83def724cfbf

kortschak · 2015-12-09T05:04:02Z

I am at a loss. That's where I am and it builds fine here.

btracey · 2015-12-09T05:04:32Z

Maybe it's a clang/ gcc thing?

btracey · 2015-12-09T05:16:22Z

Size hypothesis correct. If I change the code to

  if (LAPACKE_lsame( side, 'l' )){
            a_t = (double*)LAPACKE_malloc( sizeof(double) * lda_t * MAX(1,m) );
        }else{
            a_t = (double*)LAPACKE_malloc( sizeof(double) * lda_t * MAX(1,n) );
        }
        if( a_t == NULL ) {
            info = LAPACK_TRANSPOSE_MEMORY_ERROR;
            goto exit_level_0;
        }
        c_t = (double*)LAPACKE_malloc( sizeof(double) * ldc_t * MAX(1,n) );
        if( c_t == NULL ) {
            info = LAPACK_TRANSPOSE_MEMORY_ERROR;
            goto exit_level_1;
        }
        /* Transpose input matrices */
        if (LAPACKE_lsame( side, 'l' )){
            LAPACKE_dge_trans( matrix_layout, k, m, a, lda, a_t, lda_t );
        }else{
            LAPACKE_dge_trans( matrix_layout, k, n, a, lda, a_t, lda_t );
        }

The Dorml2 tests pass

kortschak · 2015-12-09T05:17:33Z

Nice work.

btracey · 2015-12-09T05:21:31Z

http://icl.cs.utk.edu/lapack-forum/viewtopic.php?f=13&t=4862

kortschak · 2015-12-09T05:27:19Z

Send a PR to OpenBLAS?

btracey · 2015-12-09T05:29:13Z

It looks to me like a similar problem with the failing cgo tests for the new function, Dormbr. It has

nq = LAPACKE_lsame( side, 'l' ) ? m : n;
r = LAPACKE_lsame( vect, 'q' ) ? nq : MIN(nq,k);
...
LAPACKE_dge_trans( matrix_layout, r, MIN(nq,k), a, lda, a_t, lda_t );

The documentation says that a can be any of mxk, nxk, kxm, or kxn so it doesn't seem like there is any way this code can be correct.

btracey · 2015-12-09T05:35:13Z

For dormlq
OpenMathLib/OpenBLAS#711

btracey · 2015-12-09T07:45:15Z

And yes, confirmed for Dormbr. Following code gives passing go tests.

        if( LAPACKE_lsame( vect, 'q' ) ){
          a_t = (double*)LAPACKE_malloc( sizeof(double) * lda_t * k );
        }else{
          a_t = (double*)LAPACKE_malloc( sizeof(double) * lda_t * nq );
        }

        if( a_t == NULL ) {
            info = LAPACK_TRANSPOSE_MEMORY_ERROR;
            goto exit_level_0;
        }
        c_t = (double*)LAPACKE_malloc( sizeof(double) * ldc_t * MAX(1,n) );
        if( c_t == NULL ) {
            info = LAPACK_TRANSPOSE_MEMORY_ERROR;
            goto exit_level_1;
        }
        /* Transpose input matrices */
        if( LAPACKE_lsame( vect, 'q' ) ){
          LAPACKE_dge_trans( matrix_layout, nq, k, a, lda, a_t, lda_t );
        }else{
          LAPACKE_dge_trans( matrix_layout, k, nq, a, lda, a_t, lda_t );
        }

btracey · 2015-12-09T07:50:51Z

For dormbr

OpenMathLib/OpenBLAS#713

btracey · 2015-12-09T07:53:13Z

And lapack bug

http://icl.cs.utk.edu/lapack-forum/viewtopic.php?f=13&t=4863

btracey · 2015-12-09T20:36:46Z

Responded to PR comments.

I disabled the currently failing Lapack tests with good error messages. We can reenable them when/if OpenBLAS updates. Note that this is not a cascading failure. The problem is with the c wrapper, and so functions that call Dormbr / Dormlq (i.e. SVD) through fortran get the correct answer, it's just calling these functions directly (as the test does) where the wrong answer is achieved.

btracey · 2015-12-09T20:37:05Z

PTAL

kortschak · 2015-12-10T01:42:28Z

native/dormbr.go

opts = "L", equivalent for "R". The compiler should optimise them to the same operation, but might not.

kortschak · 2015-12-10T10:37:42Z

LGTM

Add Dormbr and test

kortschak reviewed Dec 8, 2015
View reviewed changes

kortschak reviewed Dec 9, 2015
View reviewed changes

vladimir-ch reviewed Dec 9, 2015
View reviewed changes

btracey mentioned this pull request Dec 9, 2015

cgo/clapack: update codegen for LAPACKE 3.6 #75

Merged

kortschak reviewed Dec 10, 2015
View reviewed changes

btracey force-pushed the adddormbr branch from fa3ea0e to 3ae214e Compare December 10, 2015 15:10

Add Dormbr and test

69fe9ce

btracey force-pushed the adddormbr branch from 3ae214e to 69fe9ce Compare December 10, 2015 15:16

btracey added a commit that referenced this pull request Dec 10, 2015

Merge pull request #74 from gonum/adddormbr

d11e8c5

Add Dormbr and test

btracey merged commit d11e8c5 into master Dec 10, 2015

btracey deleted the adddormbr branch December 10, 2015 15:16

Add Dormbr and test #74

Add Dormbr and test #74

Uh oh!

Conversation

btracey commented Dec 8, 2015

Uh oh!

kortschak commented Dec 8, 2015

Uh oh!

btracey commented Dec 8, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

btracey commented Dec 9, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

btracey commented Dec 9, 2015

Uh oh!

kortschak commented Dec 9, 2015

Uh oh!

btracey commented Dec 9, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

btracey commented Dec 9, 2015

Uh oh!

btracey commented Dec 9, 2015

Uh oh!

kortschak commented Dec 9, 2015

Uh oh!

btracey commented Dec 9, 2015

Uh oh!

kortschak commented Dec 9, 2015

Uh oh!

btracey commented Dec 9, 2015

Uh oh!

btracey commented Dec 9, 2015

Uh oh!

kortschak commented Dec 9, 2015

Uh oh!

btracey commented Dec 9, 2015

Uh oh!

kortschak commented Dec 9, 2015

Uh oh!

btracey commented Dec 9, 2015

Uh oh!

btracey commented Dec 9, 2015

Uh oh!

kortschak commented Dec 9, 2015

Uh oh!

btracey commented Dec 9, 2015

Uh oh!

kortschak commented Dec 9, 2015

Uh oh!

btracey commented Dec 9, 2015

Uh oh!

btracey commented Dec 9, 2015

Uh oh!

btracey commented Dec 9, 2015

Uh oh!

btracey commented Dec 9, 2015

Uh oh!

btracey commented Dec 9, 2015

Uh oh!

btracey commented Dec 9, 2015

Uh oh!

btracey commented Dec 9, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!