Skip to content

ZeroDivisionError when groupby rank with method="dense" and pct=True #23864

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 11 commits into from
Dec 3, 2018
Merged

ZeroDivisionError when groupby rank with method="dense" and pct=True #23864

Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
3721987
BUG: Fixed groupby.rank w/ method="dense", pct=True if any grp count …
Koustav-Samaddar Nov 22, 2018
a1ba61e
BUG: Fixed groupby.rank w/ method="dense", pct=True
Koustav-Samaddar Nov 23, 2018
b8d343c
BUG 23666
Koustav-Samaddar Nov 23, 2018
1be7457
Fixed pep8 in the tests file
Koustav-Samaddar Nov 23, 2018
f1db09c
Made the requested modifications
Koustav-Samaddar Nov 23, 2018
7ce206c
Updated test case to assert expected result
Koustav-Samaddar Nov 23, 2018
976eccc
Moved & shrunk comment block outside if block
Koustav-Samaddar Nov 23, 2018
87ee207
Fixed pep8 formatting to have empty line at end of file
Koustav-Samaddar Nov 24, 2018
34047da
Updated to have more test cases for better coverage
Koustav-Samaddar Nov 24, 2018
88f5cb4
Made stylistic changes to code and test according to request
Koustav-Samaddar Dec 1, 2018
74f9e0b
Removed comment describing bug
Koustav-Samaddar Dec 1, 2018
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
1 change: 1 addition & 0 deletions doc/source/whatsnew/v0.24.0.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1420,6 +1420,7 @@ Groupby/Resample/Rolling
- Bug in :meth:`DataFrame.expanding` in which the ``axis`` argument was not being respected during aggregations (:issue:`23372`)
- Bug in :meth:`pandas.core.groupby.DataFrameGroupBy.transform` which caused missing values when the input function can accept a :class:`DataFrame` but renames it (:issue:`23455`).
- Bug in :func:`pandas.core.groupby.GroupBy.nth` where column order was not always preserved (:issue:`20760`)
- Bug in :meth:`pandas.core.groupby.DataFrameGroupBy.rank` with ``method='dense'`` and ``pct=True`` when a group has only one member would raise a ``ZeroDivisionError`` (:issue:`23666`).

Reshaping
^^^^^^^^^
Expand Down
5 changes: 4 additions & 1 deletion pandas/_libs/groupby_helper.pxi.in
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -588,7 +588,10 @@ def group_rank_{{name}}(ndarray[float64_t, ndim=2] out,
if out[i, 0] != out[i, 0] or out[i, 0] == NAN:
out[i, 0] = NAN
else:
out[i, 0] = out[i, 0] / grp_sizes[i, 0]
Copy link
Contributor

Choose a reason for hiding this comment

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

you need to fill with NAN if the grp_sizes[i, 0] == 0

Copy link
Contributor

Choose a reason for hiding this comment

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

make this another elif condition, then the else can be the division.

Copy link
Contributor Author

@Koustav-Samaddar Koustav-Samaddar Nov 23, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Aren't Lines 588-589 taking care of the NAN edge case?

In my testing I have only ever been able to get grp_size[i, 0] == 0 (in Line 591) in the bugged case (pct=True, method="dense").

So my idea of the fix was that grp_size[i, 0] should never be 0 in the else block and if it is, then it should be 1 (not modifying out[i, 0] would be the same as div by 1) and so there's no else block for the if on Line 590.

Copy link
Contributor

Choose a reason for hiding this comment

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

no, you need to add this case there as well, if the denominator is 0, the result is NaN

Copy link
Contributor Author

@Koustav-Samaddar Koustav-Samaddar Nov 23, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

As it is right now, doing that would cause incorrect results (would fail the test I added in test_rank.py). The reasoning behind this is:

  1. grp_size[i, 0] should legitimately be 0 only if all the values in a group are NaN. In this case the if statement in Lines 588-589 takes care of this case and therefore, will never enter the else block in question.

  2. grp_size[i, 0] shouldn't be 0 but is in the case where there is a group with only one member with a non NaN value and another index also has the same value but in a different group && method="density". In this case the correct grp_size[i, 0] should be 1 and it being 0 is what causes BUG 23666.

Therefore my fix is only performing the division by grp_size[i, 0] when it is not equal to 0 in the if block [Line 591] because the else condition only demands division by 1 (the corrected grp_size[i, 0]).

To summarise, making the result NaN on grp_size[i, 0] would stop the ZeroDivisionError from occurring in BUG 23666 and replace it with incorrect behaviour.

An additional note: the conclusion of my testing is that there is a flaw in the calculation of grp_sizes[i, 0] itself under these circumstances, but in my testing I have uncovered that there are other parameters (not just method="dense") that can have weird behaviour that is caused due to incorrect grp_sizes[i, 0]. Once, I'm able to determine the extent of this I'm going to create a new issue that would target the fixing of grp_size[i, 0] calculation bug. This fix is a stop-gap measure till then since this would correct one instance of incorrect rank behaviour.

Copy link
Member

Choose a reason for hiding this comment

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

Please improve the test case to make an assertion about the result of this expression. It’s not enough to assert that it simply computes which is the behavior now.

Doing that would aid this discussion

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah sorry, my bad. I forgot to push it.

# BUG 23666 - grp_sizes[i, 0] should never be 0
# If it's zero, it means to be 1 a.k.a. no change
if grp_sizes[i, 0] != 0:
out[i, 0] = out[i, 0] / grp_sizes[i, 0]
{{endif}}
{{endfor}}

Expand Down
15 changes: 15 additions & 0 deletions pandas/tests/groupby/test_rank.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -290,3 +290,18 @@ def test_rank_empty_group():
result = df.groupby(column).rank(pct=True)
expected = DataFrame({"B": [0.5, np.nan, 1.0]})
tm.assert_frame_equal(result, expected)


@pytest.mark.parametrize("df_dicts", [
({"A": [1, 2], "B": [1, 1]}, {"B": [1.0, 1.0]}),
({"A": [1, 1, 2, 2], "B": [1, 2, 1, 2]}, {"B": [0.5, 1.0, 0.5, 1.0]}),
({"A": [1, 1, 2, 2], "B": [1, 2, 1, np.nan]}, {"B": [0.5, 1.0, 1.0, np.nan]}),
({"A": [1, 1, 2], "B": [1, 2, np.nan]}, {"B": [0.5, 1.0, np.nan]})
])
def test_rank_zero_div(df_dicts):
Copy link
Contributor

Choose a reason for hiding this comment

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

why are you passing a dict? just pass 2 arguments in the parameterize. this is very hard to read.

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'm not sure if I understand this correctly, so I followed the same styling as a previous test in the same file.

I hope the changes are more readable!

# GH 23666
df = DataFrame(df_dicts[0])

result = df.groupby("A").rank(method="dense", pct=True)
expected = DataFrame(df_dicts[1])
tm.assert_frame_equal(result, expected)