CI: Fix flaky test_value_counts_null #32449
Conversation
pandas/tests/base/test_ops.py
Outdated
| tm.assert_series_equal(obj.value_counts(), expected) | ||
| # sort_index to avoid switched order when values share the same count | ||
| expected = expected.sort_index() | ||
| result = obj.value_counts().sort_index() |
There was a problem hiding this comment.
I think the result should just be obj.value_counts() for the test to be considered a valid test.
is it feasible for expected to be constructed to create the expected sort order or is the sort order tested elsewhere?
There was a problem hiding this comment.
I generally agree, but this is the only solution I found to work consistently on CI.
What I've found so far is the following:
- Usually,
value_countspreserves the order fromobjif multiple values share the same count - This scenario is covered by the current test design/implementation
- However, this seems to break on CI (I could only see it breaking for
float16onWindows py36_np15, but there might be more) - Locally (mac, py36) I cannot reproduce the flakiness, even when running it many times
So this might actually be a bug. I never worked with Cython, so it's hard for me to trace this deeper. It might be an issue in value_count_float64 in pandas/_libs/hashtable_func_helper.pxi
My suggestion would be to
- adjust the current code to only call
sort_indexforfloat16for now - merge this PR
- open an issue for the potential bug
By seeing if the test is still flaky afterwards we can gather more data about its relation to float16 etc.
Wdyt @simonjayhawkins?
There was a problem hiding this comment.
- sounds good with a TODO
could it be not related to float16, just duplicate values are more likely due to decreased resolution.
There was a problem hiding this comment.
I don't think so. If it's about duplicated values alone then it should always fail for repeats, see here
There was a problem hiding this comment.
This time it failed with int32 as well: https://dev.azure.com/pandas-dev/pandas/_build/results?buildId=30037&view=logs&j=3a03f79d-0b41-5610-1aa4-b4a014d0bc70&t=4d05ed0e-1ed3-5bff-dd63-1e957f2766a9&l=66
How should we go about this?
There was a problem hiding this comment.
could just revert #32281 for now instead @jbrockmendel ?
There was a problem hiding this comment.
I come to wonder if the order of values with the same count is actually deterministic. If not, is there an alternative to using sort_index on result and expected?
There was a problem hiding this comment.
could just revert #32281 for now instead @jbrockmendel ?
I think merging this PR is better than reverting the #32281. In the previous version we just skipped all tests with duplicated values. Here we at least test that the values are correct, even though we don't validate that they are ordered consistently.
| # sort_index to avoid switched order when values share the same count | ||
| result = result.sort_index() | ||
| expected = expected.sort_index() | ||
| # TODO: Order of entries with the same count is inconsistent on CI (gh-32449) |
There was a problem hiding this comment.
Is there an open issue for this?
There was a problem hiding this comment.
xref: #32514
|
@WillAyd @simonjayhawkins I added a follow-up issue and will come back to it later. Otherwise I think I addressed all comments, so this should be good to merge |
|
Thanks @SaturnFromTitan |
|
Generally fixes for flaky tests should be backported. Does the 1.0.x branch suffer from the original flakiness? |
|
no. #32281 was not backported. |
|
Great, thanks. |
As mentioned by @simonjayhawkins in #32438
Trace of the failing test:
https://dev.azure.com/pandas-dev/pandas/_build/results?buildId=29965&view=logs&j=077026cf-93c0-54aa-45e0-9996ba75f6f7&t=e95cf409-86ae-5b4d-6c5f-79395ef75e8f