-
-
Notifications
You must be signed in to change notification settings - Fork 18.3k
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
TST: Update unreliable num precision component of test_binary_arith_ops (GH37328) #37380
Conversation
Seems like a numerical precision issue indeed. Comparing the values: 16.964677878827604 and 16.964677878827608 I get the following tolerance levels: Therefore I changed: |
83d584d
to
cc04f06
Compare
I see that this issue is not limited to those values, I also get this error on #36838, where there's a mismatch between 0.8129387113107932 and 0.8129387113107933 The proposed change would catch it, but maybe we will need to relax the tolerances more to make it generic. |
@avinashpancham Merging master should fix the linting error |
xref #13339 which changed the first assert in |
@rhshadrach sorry but I dont understand what you mean. Could you maybe give some more explanation? |
@dsaxton thanks that solved it indeed |
Now that I read it again it looks more like a general comment, instead of feedback. My bad. |
CI failed due to: |
restarted. |
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.
lgtm
cc @jbrockmendel this seems reasonble to me. |
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.
pls merge master and ping on green.
# direct numpy comparison | ||
expected = self.ne.evaluate(f"nlhs {op} ghs") | ||
tm.assert_numpy_array_equal(result.values, expected) | ||
tm.assert_almost_equal(result.values, expected) |
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.
can you add a comment why we are doing this and reference to this issue
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.
yah i think this makes sense, but should have a TODO indicating that it should be equality and we had to punt
@jreback merged master and CI is greenish. Errors are memory/OS errors unrelated to this change, do we want to run this again or is this fine? |
thanks @avinashpancham |
black pandas
git diff upstream/master -u -- "*.py" | flake8 --diff