[fix] Update test_update_ops_mutation tolerance#16198
[fix] Update test_update_ops_mutation tolerance#16198sxjscience merged 1 commit intoapache:masterfrom
test_update_ops_mutation tolerance#16198Conversation
| def assert_mutate(x, y, op): | ||
| def _test_update_ops_mutation_impl(): | ||
| assert_allclose = functools.partial( | ||
| np.testing.assert_allclose, rtol=1e-10) |
There was a problem hiding this comment.
are you making it more sensitive. Will that cause more failures going forward?
I think you need different comparison function which says check for equality upto this precision -
Something like - assert_array_almost_equal
There was a problem hiding this comment.
@sxjscience @kshitij12345 sorry there was race between I writing this and Xingjian merging the change. Can you please look at this comment.
There was a problem hiding this comment.
I think we need to lower the rtol because sometimes it fails the assert_raise check
There was a problem hiding this comment.
IIUC , with the current check is if difference is less than 1e-10 , test will fail. Is this correct ?
I think it should be I don't care about 7th(or 8th precision or anything above) precision, it can go arbitrary lower than what I care about.
There was a problem hiding this comment.
We are assert_raise, which means that if the difference is larger than rtol, assert_allclose will fail and it will raise the AssertionError.
There was a problem hiding this comment.
@Vikas-kum @sxjscience Thanks for taking a look.
The new tolerance of assert_allclose (numpy doc mentions to use assert_allclose over assert_almost_equal) is now 1e-10, which means that it is more stricter now when checking for NDArray not being changed.
A consequence of this as it is used in assert_mutated is, it also allows smaller updates, i.e. -5.9604645e-08 (failure case update) to be valid change .
Thus now we have stricter check on whether any element of NDArray has changed or not (with stricter tolerance) while allowing smaller valid updates (i.e. anything above rtol of 1e-10 will be a valid update).
I think it should be I don't care about 7th(or 8th precision or anything above) precision, it can go arbitrary lower than what I care about.
(I don't know much about this but) With the instability of floating point number (rounding errors), I guess it is better to have some tolerance like 1e-10. I don't know if it is good idea to go to Machine Epsilon (lowest possible) as rtol for this.
There was a problem hiding this comment.
@kshitij12345 I think it should be safe to remove the test_mutate check.
3 participants
Fixes:
#15768 (comment)
@apeforest @sxjscience @Vikas-kum