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

ENH: Make assert_almost_equal & assert_array_almost_equal consistent. #7760

Merged
merged 1 commit into from
Jun 20, 2016

Conversation

charris
Copy link
Member

@charris charris commented Jun 18, 2016

This changes the check for scalars in assert_almost_equal so that

abs(actual - desired) < 1.5 * 10**(-decimal)

Note that the previous documentation claimed that the functions were
equivalent to

abs(actual - desired) < .5 * 10**(-decimal)

but that was not how they behaved as implemented.

Some very delicate tests may fail that did not fail before but no new failures were seen in
SciPy version 0.18.0.dev0+8346eea.

Closes #5200.

@charris
Copy link
Member Author

charris commented Jun 18, 2016

Will add release note.

@charris
Copy link
Member Author

charris commented Jun 18, 2016

Note that this leads to errors in scipy of the sort

Arrays are not almost equal to 2 decimals

(mismatch 33.3333333333%)
 x: array([ 0.25,  0.01,  0.49])
 y: array([ 0.25,  0.  ,  0.5 ])

Which is a valid failure, but the previous version let it pass.


if not issubdtype(z.dtype, number):
z = z.astype(float_) # handle object arrays

return around(z, decimal) <= 10.0**(-decimal)
return z < 10**(-decimal)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If decimal is a numpy int, this might start giving zero division errors in the future... 😈

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only after a deprecation period, and of course it would get fixed before the deprecation could land anyway...

@charris
Copy link
Member Author

charris commented Jun 18, 2016

Hmm, I had a failure for 10. **, but that was with another version. Note that I have reduced the number of changes to 1 in order to keep backwards compatibility. Scipy was spewing test errors like crazy.

@charris
Copy link
Member Author

charris commented Jun 18, 2016

Note that this only differs from #6601 in using round instead of around in the scalar case. That leaves a small window of incompatibility but otherwise a test using Decimals fails and I felt it best to maintain as much backward compatibility as possible.

EDIT: No longer the case.

@charris
Copy link
Member Author

charris commented Jun 18, 2016

Wait, I can do better...

@charris
Copy link
Member Author

charris commented Jun 19, 2016

OK, I think I'm done with this...


if not issubdtype(z.dtype, number):
z = z.astype(float_) # handle object arrays

return around(z, decimal) <= 10.0**(-decimal)
return z < 1.5 * 10.0**(-decimal)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would making this <= still make sense and remove the possibly stronger tests? Not sure if that makes sense or not. Anyway, looks good to me.
I would like to nudge you towards adding a test ;p. Wondering if we can somehow make it easier to understand what "decimal" means in the documentation. Also "vagaries" seems a bit difficult to me as a non-native speaker, maybe "details"?

Copy link
Member Author

@charris charris Jun 19, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had that, but changed it on account of

nt.assert_almost_equal([1.5], [0], decimal=0)

failing in master due to rounding the difference 1.5 up. Note that

nt.assert_almost_equal([1.49999], [0], decimal=0)

does pass (just to assure everyone the 1.5 is the proper factor).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can't make everything exactly the same, but I figure that any problems are in tests too close to the edge in any case.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah need a test. If there had been one in the first place we would never have ended up with 1.5 * 10**(-decimal). That was due to the use of a <= instead of <, and I think that came about from working around some other perceived bug. I'd just change it if it didn't cause so many new failures in downstream code.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Test added.

This changes the check for scalars in assert_almost_equal so that

    abs(actual - desired) < 1.5 * 10**(-decimal)

Note that the previous documentation claimed that the functions were
equivalent to

    abs(actual - desired) < .5 * 10**(-decimal)

but that was not how they behaved in practice.

Due to the change in implementation, some very delicate tests may fail
that did not fail before. No extra failures were noted in scipy.

Closes numpy#5200.
@mhvk
Copy link
Contributor

mhvk commented Jun 20, 2016

Looks all OK to me.

@charris charris merged commit f902cfd into numpy:master Jun 20, 2016
@charris charris deleted the fix-almost-equal-tests branch August 28, 2016 20:17
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.

None yet

4 participants