gonum / blas

A BLAS implementation for Go [DEPRECATED]
172 stars 16 forks source link

Cgo drnm2 panics with zero-length slice #133

Closed btracey closed 9 years ago

btracey commented 9 years ago

Dnrm2 allows a zero-length slice, but cgo does not check for zero length before taking &x[0].

kortschak commented 9 years ago

This will be true of all the functions that take an n and a vector. I can add a check for n == 0 before the cgo call and return early then. If the ret type is int, return -1; this catches I*amax. The native implementations have some potential panics prior to the equivalent test, so presumably I should mimic that, but what is the basis for those?

btracey commented 9 years ago

I'm not sure what you mean by "the basis for those". In Idamax right now the code is:

if n < 2 {
        if n == 1 {
            return math.Abs(x[0])
        }
        if n == 0 {
            return 0
        }
        if n < 1 {
            panic(negativeN)
        }
    }

It returns 0 and not -1 because the refernce BLAS returns 1 when the size is zero.

btracey commented 9 years ago

Sorry, BLAS returns 1 for Idamax when the slice length is zero, and also returns 0 when the slice is zero for Dnrm2.

btracey commented 9 years ago

Easy to change if we'd rather it return -1. As far as I'm concerned, as long as we're doing something reasonable and cgo and native have the same behavior, I'm happy to deviate from the strict blas standard.

kortschak commented 9 years ago

There are panics for incX < 0 when n == 0.

btracey commented 9 years ago

Yes.

Also, sorry, I misread the code. LAPACK does return an invalid index when n == 0.

kortschak commented 9 years ago

OK, so what would you like. I have the hooks for int, float and none - in the float64 case we return 0, for int?

(What on earth is the basis for returning 1 when n == 0?).

btracey commented 9 years ago

I think Idamax should return -1. This is consistent with the current behavior and the reference standard. (Reference returns 0, but it's one-indexed)