BUG: drop_duplicates drops non-duplicate rows in the presence of integer columns
State: closed

Fixes GH #11376


Labels:
name: Bug
name: Reshaping
description: Concat, Merge/Join, Stack/Unstack, Explode

Comments:
plz do not modify `groupby.py:get_group_index` function. that function has nothing to do with https://github.com/pydata/pandas/issues/11376 and does not need any fixing

yeh I think Ideally we'd like to get the perf benefit on not factororizing integers if possible, but seems like reverting #10917 (though with @evanpw ) tests would be the ideal

This PR has the perf benefit without the bug, so why revert #10917 instead?

@evanpw `get_group_index` should only get factorized values; you are giving it wrong values then trying to fix it by factorizing inside the function!

can you plz just revert https://github.com/pydata/pandas/pull/10917 and not change `get_group_index`?

`get_group_index` doesn't _need_ factorized values, though. All it needs is non-negative integers in a known range. The situation where a column needs to be factorized to make progress is a rare edge case that only happens when the difference between the largest and smallest integers in the column is comparable in magnitude to INT64_MAX.

`get_group_index` actually has a beautiful mathematical description. It takes a subset `U` of the group `Z_n1 x Z_n2 x ... Z_nm` and maps it to the corresponding subset of `Z_{n1 * n2 * ... * nm}` (i.e., a list of numbers) via the usual isomorphism (the one you get by sorting the former group lexicographically). It doesn't matter whether or not `U` projects down to the entire space `Z_ni` for every `i` (i.e., it doesn't matter whether every column is factorized). It still works, as long as you're careful about integer overflow.

@evanpw before trying to explain what the function does, can you plz check [commit history of the function code](https://github.com/behzadnouri/pandas/blame/b09b84e8e0baf89e78b618cdda30af11087d2e4a/pandas/core/groupby.py#L3723) and see who had contributed to its code?

you are _modifying_ the function code to _fix_ it, then you say that it does not need factorized labels!

that function does a core calculation to many pandas functionality, and has nothing to do with the bug you are trying to close.

@behzadnouri 

I see no problem with @evanpw trying a different soln. If we can assure that it does the correct thing, but maybe in a different way to keep the perf benefits, where is the harm?

@jreback 

you are the one who has done the harm, namely https://github.com/pydata/pandas/pull/10917

obviously you do not understand how these functions work. can u plz at least not be dismissive when i am trying to help you.

@behzadnouri of course I did this change. yet we didn't have enough test coverage or it wouldn't have been ok.

I am all for rolling this back. but @evanpw soln seems reasonable. 

pls don't be dismissive of others, it is simply not nice.

@jreback get_group_index has nothing to do with https://github.com/pydata/pandas/issues/11376 bug

can we plz not modify that function? that function should only work with factorized labels! you see that this PR is modifying it to _fix_ it because it is calling the function with non-factorized labels.

@behzadnouri see that would be a MUCH better comment. thank you.

I agree we should be clear on whether things are factorized or not going into functions.

So maybe the doc-string needs to be updated to make that clear.

@evanpw  please consider these two cases

```
import pandas as pd
import numpy as np
_INT64_MAX = np.iinfo(np.int64).max
df_e = pd.DataFrame([[0]*63, [_INT64_MAX-1]*63]).drop_duplicates()
df_w = pd.DataFrame([[0]*63, [_INT64_MAX]*63]).drop_duplicates()
```

@behzadnouri Sorry if I came across as rude. I really like the function you've written! I just happened to notice that it also works perfectly for non-factorized inputs, except in a certain edge case involving integer overflow (and already has to handle non-factorized intermediate values in some cases), so that we could keep the performance improvement from jreback's change while fixing the bug. I personally have no pressing need for `drop_duplicates` to be super fast, so if this change is going to be controversial, then let's just revert #10917.

@kawochen Good catch. Should be fixed now.

ok, @evanpw pls revert #10917. but KEEP your tests, I guess that is how this slipped thru.

Should I do it in a separate PR?

@evanpw no you can do it here. separate your tests into a single commit, then you'll will need to fix up some stuff on the revert (e.g. the whatsnew note). so you'll end up with the revert commit with some conflicts fixed and your tests in a commit. you will also need a new whatsnew note for 0.17.1 (which you can put in either)

This wiped out the whatsnew message from 0.17.0. I'm not sure if that's the right thing in this case.

@evanpw no, leave the whatsnew from 0.17.0 (and add a new one for 0.17.1), leave the asv benchmark as well.

Okay, this is at the very limit of my git ability, but it should be correct now.

can you add back the asv benchmark? (you can just add it in and combine with say the tests commit)

It should still be there. I only reverted the changes to frame.py.

ahh, right....hahah ok then.

ping on green.

all green

thanks!