Changed scal to overwrite NaN with alpha = 0

[coveralls]

Coverage increased (+0.05%) when pulling 2f2377c74c30f5f233dd424da1df4f58b9319a31 on fixzero into a60c9e1ee40c0267eb5a10ac9b60895b8bd86863 on master.

[btracey]

All of the tolEqual functions should probably be changed to tolSame, but I wanted to open this because more weirdness is discovered. Apparently Idamax (at least in Accelerate) treats any value as > NaN. So, for the vector []float64{math.NaN(), 2}, idamax is 1 rather than 0. The go implementation does the same tests as the reference. Do other cblas implementations think that 2 > NaN?

[kortschak]

package main

import (
    "fmt"
    "math"

    "github.com/gonum/blas/cblas"
)

func main() {
    x := []float64{math.NaN(), 2}
    fmt.Println(cblas.Blas{}.Idamax(len(x), x, 1))
}

$ go run idamax.go 
0

but

package main

import (
    "fmt"
    "math"

    "github.com/gonum/blas/cblas"
)

func main() {
    x := []float64{2, math.NaN()}
    fmt.Println(cblas.Blas{}.Idamax(len(x), x, 1))
}

$ go run idamax.go 
0

Using OpenBLAS.

[btracey]

package main

import (
    "fmt"
    "math"

    "github.com/gonum/blas/cblas"
)

func main() {
    x := []float64{math.NaN(), 2}
    fmt.Println(cblas.Blas{}.Idamax(len(x), x, 1))
    x = []float64{2, math.NaN()}
    fmt.Println(cblas.Blas{}.Idamax(len(x), x, 1))
}

brendan:~/Documents/mygo/src/Testing/idanan$ go run idanan.go 1 0

This is on OSX Accelerate. The OpenBLAS behavior is what I would expect. 2 is not greater than NaN, and NaN is not greater than 2, so no element is larger than the first. Why Accelerate is different I have no idea. It would be one thing if it kept the NaN, but intentionally silencing the NaN is weird.

[btracey]

I found this thread [0] which suggests previous weirdness with Idamax. I also found [1] where the documented behavior is the exact opposite of Accelerate behavior.

[0] http://grouper.ieee.org/groups/754/email/msg00610.html [1] http://docs.oracle.com/cd/E19059-01/fortec6u2/806-7993/idamax.html

[kortschak]

So this depends on our decision wrt NaN treatment. Do we want to discuss that further? If we are going with 0. * NaN == 0. for the kinds of cases mtj was describing in his reply to us then this LGTM.

[btracey]

I think MTJ's behavior makes sense with what I've read from him and about BLAS. The specific case of Idamax, I think either this behavior is the right behavior, or it should return the index of a NaN.

[kortschak]

LGTM

[fhs]

0. * NaN == 0 disagrees with reference implementation:

$ CGO_LDFLAGS=-lcblas go test -test.run Dscal
--- FAIL: TestDscal (0.00s)
    level1double.go:1745: dscal: mismatch NaN, : expected [0 0], found [NaN 0]
    level1double.go:1745: dscal: mismatch NaNInc, : expected [0 NaN 0], found [NaN NaN 0]
FAIL

Maybe the builder should test against the reference implementation?

[btracey]

I think this has to be the fault of the reference implementation. It disagrees with OpenBLAS and with Accelerate, and it’s internally inconsistent. 0 should not mean “remove NaN except in Scal function"

On Jan 3, 2015, at 8:21 AM, Fazlul Shahriar notifications@github.com wrote:

1. * NaN == 0 disagrees with reference implementation:

$ CGO_LDFLAGS=-lcblas go test -test.run Dscal --- FAIL: TestDscal (0.00s) level1double.go:1745: dscal: mismatch NaN, : expected [0 0], found [NaN 0] level1double.go:1745: dscal: mismatch NaNInc, : expected [0 NaN 0], found [NaN NaN 0] FAIL Maybe the builder should test against the reference implementation?

— Reply to this email directly or view it on GitHub https://github.com/gonum/blas/pull/65#issuecomment-68594720.