Skip to content

BUG: Ignore differences in NAN for computing ULP differences #15598

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

Merged
merged 3 commits into from
Feb 19, 2020

Conversation

r-devulap
Copy link
Member

Fixes #15593

@r-devulap r-devulap force-pushed the ignore-differences-nan branch from 02f413d to 8047edd Compare February 18, 2020 19:46
@r-devulap r-devulap changed the title TST: Ignore differences in NAN for umath accuracy tests BUG: Ignore differences in NAN for computing ULP differences Feb 18, 2020
@r-devulap r-devulap force-pushed the ignore-differences-nan branch from 8047edd to 0e51583 Compare February 18, 2020 19:50
@seberg
Copy link
Member

seberg commented Feb 18, 2020

Thanks, do you think it might make sense to push it even further down to the integer_representation function? Since the "signed magnitude" representation is not just a view as far as I understand it, that might make sense?

@r-devulap
Copy link
Member Author

not so sure about that, it is the max ULP diff function that doesn't want to differentiate between NAN's. The integer_representation function simply returns the binary representation of a floating point and I think that should stay true to its intent for NAN's as well.

@r-devulap
Copy link
Member Author

x[np.argwhere(np.isnan(x))] = np.nan fails with E IndexError: too many indices for array for a single element array like x = np.array(1.2) Any suggestions on how to elegantly fix it without special casing it?

@seberg
Copy link
Member

seberg commented Feb 18, 2020

@r-devulap any reasons you are using np.argwhere at all, just remove it? Not sure I agree with the interpretation of integer_representation, but it also does not really matter much right now.

@r-devulap r-devulap force-pushed the ignore-differences-nan branch from 0e51583 to 8f25b48 Compare February 18, 2020 21:26
@r-devulap
Copy link
Member Author

@r-devulap any reasons you are using np.argwhere at all, just remove it?

Thanks :)

sine and cosine of large numbers are always computed via the standard C
library and the results vary depending on the platform.
@seberg
Copy link
Member

seberg commented Feb 19, 2020

@r-devulap looks good to me, would you be up for adding a test as well, since I think the ulp thing is arguably a bug fix in our testing utils, and also testing utils need testing :).

@r-devulap
Copy link
Member Author

Sure, what do you have in mind? ensure ulp error between two different representations of NAN = 0?

@r-devulap
Copy link
Member Author

@seberg let me know if that suffices.

@seberg
Copy link
Member

seberg commented Feb 19, 2020

Test would be nicer if you move it into numpy/testing/tests/test_utils.py. Otherwise looks good to me, I will let Matti sign off on the line removal changes and merge. Thanks!

@mattip
Copy link
Member

mattip commented Feb 19, 2020

I think there is more to do with the umath_accuracy test but this solves the current issues. xref gh-15605 for some possible improvements

@mattip mattip merged commit 760800d into numpy:master Feb 19, 2020
@mattip
Copy link
Member

mattip commented Feb 19, 2020

Thanks @r-devulap for the quick response

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

Successfully merging this pull request may close these issues.

BUG: test_umath_accuracy tests for a specific NAN
4 participants