Skip to content

BUG: Avoid duplicates in DatetimeIndex.intersection #38196

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.

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 3 commits into from
Dec 2, 2020

Conversation

phofl
Copy link
Member

@phofl phofl commented Nov 30, 2020

  • tests added / passed
  • passes black pandas
  • passes git diff upstream/master -u -- "*.py" | flake8 --diff

@jbrockmendel I think we should call _convert_can_do_setop here too, but

def test_intersection_list(self):
tests the opposite

@@ -687,7 +687,7 @@ def intersection(self, other, sort=False):
self._validate_sort_keyword(sort)
self._assert_can_do_setop(other)

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

Choose a reason for hiding this comment

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

should we handle the self.equals(other) but non-unique case here? (like MultIIndex i think)

Copy link
Member Author

Choose a reason for hiding this comment

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

Should be consistent. best would probably be to handle this here.

Copy link
Member Author

Choose a reason for hiding this comment

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

Handled it here now, and called _convert_can_do_setop. Have seen you moved the remaining part to _intersection in #38190, so I will keep this here as it is now that we are consistent

pd.Timestamp("2019-12-12"),
]
)
result = idx1.intersection(idx1)
Copy link
Member

Choose a reason for hiding this comment

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

sort?

Copy link
Member Author

Choose a reason for hiding this comment

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

Hm good point. Parametrized the test

@jbrockmendel
Copy link
Member

I think we should call _convert_can_do_setop here too, but [test_intersection_list] tests the opposite

yah im pretty sure that test is wrong xref #38111

@phofl
Copy link
Member Author

phofl commented Dec 1, 2020

Fixed the test

@jreback jreback added the Refactor Internal refactoring of code label Dec 2, 2020
@jreback jreback added this to the 1.2 milestone Dec 2, 2020
@jreback jreback merged commit b2f6b70 into pandas-dev:master Dec 2, 2020
@jreback
Copy link
Contributor

jreback commented Dec 2, 2020

thanks @phofl very nice

@phofl phofl deleted the datetimindex_intersection branch December 2, 2020 12:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Refactor Internal refactoring of code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants