Make goblas the default BLAS implementation #63
Conversation
kortschak
force-pushed
the
goblas
branch
2 times, most recently
from
3d92d42
to
5bd8062
Compare
Also allow cblas use in testing with `go test -tags cblas`.
|
It appears that it fails on FAIL: dense_test.go:551: S.TestPow. I get the same failure on my machine. Is there an issue with Mul? |
|
This change highlights two issues.
|
|
I think I know why this is happening, and I think it shows that cblas is actually the one that is incorrect. In goblas, @btracey (correctly) does C <- aAB + bC, which means that if (as is the case in this test) C is full of NaN, C is NaN at completion. Why this doesn't happen in cblas? Dunno. |
| //+build cblas | ||
|
|
||
| package mat64 | ||
|
|
There was a problem hiding this comment.
Does this mean the package is tested with cblas? Is this file somehow avoided with the go test call?
There was a problem hiding this comment.
If you want to test with cblas, use go test -tags cblas. This does not happen on travis. If you do this locally, it will fail at the moment - we need to harmonise panic values between the two implementations. My proposal would be to drop the c from the beginning of the cblas panics and use those in both - the reasoning being that those messages are more informative (they give a stronger hint to which parameter was incorrect).
There was a problem hiding this comment.
Use the cblas panic messages rather than the goblas ones? SGTM.
I see what this code does now. Thanks. I hadn't used build tags before.
|
LGTM. Now that we have a default implementation registered, do we need the explicit nil checks in functions? We could instead move the nil check into register and then it's impossible for it to be nil. Doesn't need to happen here, just a note for the future. |
|
Would we also want to have downstream packages (stat, for example) use build tags for goblas vs cblas? |
|
I'm under the impression this is a temporary tie-over until there is a single place to register BLAS (located in the blas package). Then there is one tag for all code whether you're interfacing with BLAS directly, mat64 indirectly, or something else. |
|
LGTM |
|
PTAL |
|
LGTM as well. |
The tag only specifies the mat64 test behaviour; I don't want to use tags anywhere else for specifying which implementation, that's down to client code, but yes, once we've centralise the registration, a single Register call will do all the work of changing implementation. |
| @@ -501,7 +501,7 @@ func (s *S) TestMul(c *check.C) { | |||
|
|
|||
| // These probably warrant a better check and failure. They should never happen in the wild though. | |||
| temp.mat.Data = nil | |||
| c.Check(func() { temp.Mul(a, b) }, check.PanicMatches, "cblas: index of c out of range", check.Commentf("Test %d")) | |||
| c.Check(func() { temp.Mul(a, b) }, check.PanicMatches, "general: insufficient length", check.Commentf("Test %d")) | |||
There was a problem hiding this comment.
Should this panic message be "blas: index out of range" to match https://github.com/gonum/blas/pull/66/files
There was a problem hiding this comment.
Not yet. When goblas is being harmonised this should change too.
|
LGTM |
Make goblas the default BLAS implementation
Also allow cblas use in testing with
go test -tags cblas.