Skip to content

CI: Fix flaky test_value_counts_null #32449

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.

Sign up for GitHub

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

Merged
merged 5 commits into from
Mar 7, 2020
Merged

CI: Fix flaky test_value_counts_null #32449

Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
2113521
sorting by index before comparing results in test_value_counts_null
SaturnFromTitan Mar 4, 2020
cde9df6
using sort_index for value_counts tests only for float16
SaturnFromTitan Mar 5, 2020
f04d255
Merge branch 'master' into fix-flaky-tests-test_value_counts_null
SaturnFromTitan Mar 5, 2020
ab04a73
using sort_index for all fixture values to handle all inconsistencies…
SaturnFromTitan Mar 5, 2020
72b4da1
only using sort_index if there are duplicated values
SaturnFromTitan Mar 5, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 9 additions & 2 deletions pandas/tests/base/test_ops.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -344,13 +344,20 @@ def test_value_counts_null(self, null_obj, index_or_series_obj):
expected = pd.Series(dict(counter.most_common()), dtype=np.int64)
expected.index = expected.index.astype(obj.dtype)

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()
Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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_counts preserves the order from obj if 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 float16 on Windows 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

  1. adjust the current code to only call sort_index for float16 for now
  2. merge this PR
  3. 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?

Copy link
Member

Choose a reason for hiding this comment

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

  1. sounds good with a TODO

could it be not related to float16, just duplicate values are more likely due to decreased resolution.

Copy link
Contributor Author

@SaturnFromTitan SaturnFromTitan Mar 5, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I don't think so. If it's about duplicated values alone then it should always fail for repeats, see here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

could just revert #32281 for now instead @jbrockmendel ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

tm.assert_series_equal(result, expected)

# can't use expected[null_obj] = 3 as
# IntervalIndex doesn't allow assignment
new_entry = pd.Series({np.nan: 3}, dtype=np.int64)
expected = expected.append(new_entry)
tm.assert_series_equal(obj.value_counts(dropna=False), expected)

# sort_index to avoid switched order when values share the same count
expected = expected.sort_index()
result = obj.value_counts(dropna=False).sort_index()
tm.assert_series_equal(result, expected)

def test_value_counts_inferred(self, index_or_series):
klass = index_or_series
Expand Down