Skip to content

BUG: Fix duplicates in intersection of multiindexes #36927

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 27 commits into from
Nov 29, 2020
Merged

BUG: Fix duplicates in intersection of multiindexes #36927

Changes from 1 commit
Commits
Show all changes
27 commits
Select commit Hold shift + click to select a range
cdefaae
Fix duplicates in intersectin of multiindexes
phofl Oct 6, 2020
fbd63f2
Fix duplicates in index intersection
phofl Oct 6, 2020
53a37d1
Modify test and avoid None issues
phofl Oct 6, 2020
5675a4e
Fix failing test
phofl Oct 6, 2020
134936c
Merge branch 'master' of https://github.com/pandas-dev/pandas into 36915
phofl Oct 9, 2020
582c0b9
Change comment
phofl Oct 9, 2020
7805de5
Add unique after intersection
phofl Oct 11, 2020
67691df
Merge branch 'master' of https://github.com/pandas-dev/pandas into 36915
phofl Oct 11, 2020
8fb0055
Merge branch '31326' into 36915
phofl Oct 11, 2020
66b519f
Fix merge bug
phofl Oct 11, 2020
cb1477b
Add tests and whatsnew
phofl Oct 11, 2020
0fb2561
Add rename
phofl Oct 26, 2020
3c19d57
Merge branch 'master' of https://github.com/pandas-dev/pandas into 36915
phofl Oct 26, 2020
10524fd
Fix check in merge operation
phofl Oct 26, 2020
3dde0ee
Exit set ops when nonunique
phofl Nov 12, 2020
a0a1a33
Merge branch 'master' of https://github.com/pandas-dev/pandas into 36915
phofl Nov 12, 2020
45dfb84
Merge branch 'master' of https://github.com/pandas-dev/pandas into 36915
phofl Nov 22, 2020
d71a499
Roll back to initial version
phofl Nov 22, 2020
d873d5a
Change whatsnew
phofl Nov 22, 2020
e90239a
Move whatsnew
phofl Nov 27, 2020
c2b448a
Merge branch 'master' of https://github.com/pandas-dev/pandas into 36915
phofl Nov 27, 2020
742716e
Change gh reference
phofl Nov 27, 2020
321797a
Remove pd
phofl Nov 27, 2020
a980ec0
Remove whatsnew from 1.2
phofl Nov 28, 2020
972fd48
Fix test
phofl Nov 28, 2020
fe1ded4
Make condition more clear and add assert
phofl Nov 28, 2020
8e4d47b
Use shape for equality check
phofl Nov 28, 2020
File filter

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Add rename
  • Loading branch information
@phofl
phofl committed Oct 26, 2020
commit 0fb25612cc66c139cbd8d73e2ec0d9e13b62fbb5
2 changes: 1 addition & 1 deletion pandas/core/indexes/multi.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3476,7 +3476,7 @@ def intersection(self, other, sort=False):

if self.equals(other):
if self.has_duplicates:
return self.unique()
return self.unique().rename(result_names)
Copy link
Member

@jbrockmendel jbrockmendel Nov 30, 2020

Choose a reason for hiding this comment

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

@phofl it looks like we do something slightly different in many of the FooIndex.intersection methods in the self.equals(other) case:

# Index (and IntervalIndex pending #38190) if self.equals(other) and not self.has_duplicates: return self._get_reconciled_name_object(other) # datetimelike if self.equals(other): return self._get_reconciled_name_object(other) # MultiIndex if self.equals(other): if self.has_duplicates: return self.unique().rename(result_names) return self._get_reconciled_name_object(other) # PeriodIndex if self.equals(other): return self._get_reconciled_name_object(other) # RangeIndex if self.equals(other): return self._get_reconciled_name_object(other)

The RangeIndex one is equivalent to the Index/Intervalindex bc it never has duplicates (can add that check to make the code match exactly). Can the others be made to have identical logic?
Copy link
Member Author

@phofl phofl Dec 1, 2020

Choose a reason for hiding this comment

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

Is there still an open case where I can be of help?
Copy link
Member

@jbrockmendel jbrockmendel Dec 1, 2020

Choose a reason for hiding this comment

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

I think we're all set for now, will take another look after all the current Index.intersection PRs go through. thanks
Copy link
Member Author

@phofl phofl Dec 1, 2020

Choose a reason for hiding this comment

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

Great
Copy link
Member

@jbrockmendel jbrockmendel Dec 2, 2020

Choose a reason for hiding this comment

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

It looks like several of them are still either

 if self.equals(other): return self._get_reconciled_name_object(other)

which i think is wrong if it has duplicates, or

 if self.equals(other) and not self.has_duplicates: return self._get_reconciled_name_object(other)

which isnt handling the has-duplicates case like the others. can these all be identical?

Also if you really get on a roll, DatetimeTimedeltaMixin._intersection has cases for len(self) == 0 and len(other)==0 that would be nice to standardize+test. RangeIndex._intersection has a similar check.
Copy link
Member Author

@phofl phofl Dec 2, 2020

Choose a reason for hiding this comment

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

Will look through them and try to standardize them as much as possible
return self.rename(result_names)

if not is_object_dtype(other.dtype):
Expand Down