Skip to content
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

Jump to bottom

REF: Implement isin on DTA instead of DTI #38012

Merged
merged 3 commits into from
Nov 24, 2020
Merged

REF: Implement isin on DTA instead of DTI #38012

merged 3 commits into from
Nov 24, 2020

Conversation

jbrockmendel
Copy link
Member

No description provided.

jreback
jreback reviewed Nov 23, 2020
View reviewed changes
Copy link
Contributor

@jreback jreback left a comment

Choose a reason for hiding this comment

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

looks reasonable, pre commit failing

@jreback jreback added Refactor Internal refactoring of code Indexing Related to indexing on series/frames, not to indexes themselves labels Nov 23, 2020
jorisvandenbossche
jorisvandenbossche reviewed Nov 23, 2020
View reviewed changes
Copy link
Member

@jorisvandenbossche jorisvandenbossche left a comment

Choose a reason for hiding this comment

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

Why add it to the array class while for none of our other EAs it is implemented on the array?

(now, this could also be an indication we should think about adding isin more generally)

@jbrockmendel
Copy link
Member Author

Why add it to the array class while for none of our other EAs it is implemented on the array?

  1. it is implemented on Categorical
  2. jreback requested the move in BUG: isin incorrectly casting ints to datetimes #37528
  3. i intend to de-duplicate code in isin, equals, and _validate_listlike

@jorisvandenbossche
Copy link
Member

OK, sounds good

@jreback jreback added this to the 1.2 milestone Nov 24, 2020
@jreback jreback merged commit 24e881d into pandas-dev:master Nov 24, 2020
@jreback
Copy link
Contributor

jreback commented Nov 24, 2020

thanks @jbrockmendel

yeah I am +1 on adding .isin too the base class (and coercion would go thru _from_sequence so likely this would mostly be a base-class method (not much need to override)

@jbrockmendel jbrockmendel deleted the ref-isin branch November 24, 2020 03:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jreback jreback jreback left review comments

@jorisvandenbossche jorisvandenbossche jorisvandenbossche left review comments

Assignees
No one assigned
Labels
Indexing Related to indexing on series/frames, not to indexes themselves Refactor Internal refactoring of code
Projects
None yet
Milestone
1.2
Development

Successfully merging this pull request may close these issues.
3 participants
@jbrockmendel @jorisvandenbossche @jreback