Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Incorrect Idamax with NaN values #624

Closed
btracey opened this issue Aug 25, 2015 · 9 comments
Closed

Incorrect Idamax with NaN values #624

btracey opened this issue Aug 25, 2015 · 9 comments
Labels

Comments

@btracey
Copy link
Contributor

btracey commented Aug 25, 2015

In the reference implementation, the arrays [5, +inf, NaN, 8, 9] and [5, NaN, +inf, 8, 9] have an Idamax of 2 and 3 respectively. OpenBLAS returns 2 and 2. While this doesn't strictly have to be a bug (since the behavior with NaN isn't specified), it seems like x[idamax(x)] shouldn't depend on slice order.

@brada4
Copy link
Contributor

brada4 commented Dec 2, 2015

NaN is not comparable. You should get floating point exception on NaN comparison and undefined result. correct answer is a guess from list of all NaNs in array and idamax of the rest. If you read idamax documentation...

@btracey
Copy link
Contributor Author

btracey commented Dec 2, 2015

Idamax documentation (http://www.netlib.org/lapack/explore-html/dd/de0/idamax_8f_source.html): "IDAMAX finds the index of the first element having maximum absolute value." It does not specify behavior when NaN is present.

@brada4
Copy link
Contributor

brada4 commented Dec 2, 2015

With NaN not being comparable it can be bigger than infinity. Why are you insisting on consistency on undefined behaviour?
Especially the order of comparison just cannot happen with parallel and vectorized BLAS.

@brada4
Copy link
Contributor

brada4 commented Dec 29, 2015

inf.gt.NaN gives different results on different compilers and different flags (like gfortran fast-math vs i387 vs SSE2 vs intel vs pgi) - do you expect most popular case is correct or idamax of version numbers should be winning case?

Solution for your expectation is to wrap the idamax function and replace NaNs with zeroes.
As it stands today idamax has no chance to return error conditions like seeing nans

in real life you need to rewind your computation back to origins and NOT create NaNs first hand.

@xianyi
Copy link
Collaborator

xianyi commented Dec 29, 2015

@brada4 , thank you for the update. So far, OpenBLAS is very hard to deal with NaN.

@martin-frbg
Copy link
Collaborator

Implementation compatibility may make little sense when it comes to undefined behaviour, so how about maximum execution speed. Anybody know the most efficient way to identify NaN values in assembly (for x86 I am led to believe it is ucomisd) ? Even if the OpenBLAS *amax implementations elected to return the highest possible index number immediately upon encountering the first NaN element, it would solve #671, #723 without being "wrong". The alternative would appear to be to clearly document that OpenBLAS intentionally relies on numerically valid input.

@brada4
Copy link
Contributor

brada4 commented Dec 31, 2015

In case of NaN-s in (netlib) *max and very lucky compiler and fpu combination it can happen NaN >= Inf, and in the next step 1e-99 >= NaN, and it is exactly what undefined behaviour means.

Sure it can be optimized to common sense that first infinity or Nan is maximum and dont search further, but then again bug report implies NaNs should be replaced with fake zeroes.

NaN check before LAPACK calls is only (optionally) present in LAPACKE, and not before that. Which means that silent garbage in garbage out is very well within specification.

@martin-frbg
Copy link
Collaborator

GIGO is one thing, crashing is another. My suggestion is either strive for the fastest non-crashing implementation no matter if its result in the NaN case happens to coincide with netlib-du-jour, or document that OpenBLAS trades NaN safety for speed.

@brada4
Copy link
Contributor

brada4 commented Dec 31, 2015

This one is not crashing, it is just confused by NaN in the way different from reporter's fortran compiler.
"BLAS predates IEEE 754 and does not have any provisions for concept of NaN in most cases"

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants