-
-
Notifications
You must be signed in to change notification settings - Fork 25.8k
[MRG] TST Adds atol to test_dtype_match #14385
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
[MRG] TST Adds atol to test_dtype_match #14385
Conversation
The failure disappears? See latest cron job https://travis-ci.org/scikit-learn/scikit-learn/builds/559440889 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @thomasjpfan !
The failure disappears?
I am still seeing it occasionally in PRs, e.g.,
Mismatch: 20%
E Max absolute difference: 7.70952898e-06
E Max relative difference: 0.00015937
E x: array([-0.033526, -0.2883 , -0.158729, 0.094338, 0.378415],
E dtype=float32)
E y: array([-0.03352 , -0.288305, -0.158737, 0.094331, 0.378419])
I can't see any randomness in the test? |
I agree there shouldn't be, that's was en empirical observation (or maybe something in the dependencies that got an update) :) We can certainly investigate, but for now I'm mostly concerned about fixing unrelated test failures in the PRs I'm working on.
That's a generally question about how do we choose the atol / rtol threshold. Running the test with multiple random states, and checking that it doesn't fail is a strong indicator. In the few failures I encountered, the needed atol error was 7e-6 two times, while here it's 5-e4. It's a small sample size, but I think it should be enough. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, a small atol seems reasonable.
Does this affect 0.21? Should the fix be included there?
|
I think so. When releasing 0.21, we only skip this test on MAC OS, but this failure happens on Linux. |
Reference Issues/PRs
Fixes #14219
What does this implement/fix? Explain your changes.
Adds
atol
when comparing float32 and float4.