Skip to content

CLN: Use ABC classes where possible #29020

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
Closed
wants to merge 1 commit into from
Closed

CLN: Use ABC classes where possible #29020

wants to merge 1 commit into from

Conversation

kgoehner
Copy link

@kgoehner kgoehner commented Oct 16, 2019 •
edited
Loading

@jbrockmendel
Copy link
Member

Looks like the linter doesn't like the import order. can you run isort pandas/core/reshape/concat.py

@TomAugspurger
Copy link
Contributor

This will need to be profiled carefully given @jorisvandenbossche's comments in #27353 (comment).

WillAyd
WillAyd requested changes Oct 16, 2019
View reviewed changes
Copy link
Member

@WillAyd WillAyd left a comment

Choose a reason for hiding this comment

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

Looks good generally. w.r.t. performance none of these look like they are in tight loop constructs so I think OK

WillAyd
WillAyd requested changes Oct 17, 2019
View reviewed changes
pandas/core/indexes/base.py Outdated
@@ -353,7 +353,7 @@ def __new__(
return Index(data, dtype=object, copy=copy, name=name, **kwargs)

# index-like
elif isinstance(data, (np.ndarray, Index, ABCSeries)):
elif isinstance(data, (np.ndarray, ABCIndexClass, ABCSeries)):
Copy link
Member

Choose a reason for hiding this comment

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

Can you limit scope of change in this module to just ABCExtensionArray? I think that's the only one that can be exclusively replaced with the ABC... import

The rest let's leave separate

@WillAyd
Copy link
Member

WillAyd commented Oct 18, 2019

lgtm can you merge master and try again? I think should resolve CI

@kgoehner
Copy link
Author

Rebased, we'll see how this goes.

@jreback jreback added the Clean label Oct 18, 2019
jreback
jreback requested changes Oct 18, 2019
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.

this looks ok, can you run the asv suite (take a while) and see if this has any perf impact, you might want to start with a small subset (maybe the reshaping asvs)

@kgoehner
Copy link
Author

@jreback When comparing 2ce2f4f and 9f03837 there is no significant difference on the reshaping benchmark.

reshaping-results.zip

@WillAyd
Copy link
Member

WillAyd commented Oct 22, 2019

@Kazz47 do you have bash output available you can post instead? Would be easier to review

@WillAyd
Copy link
Member

WillAyd commented Nov 7, 2019

@Kazz47 can you merge master and fix conflict? Also for asv run output just post the output from your terminal

@WillAyd
Copy link
Member

WillAyd commented Dec 17, 2019

Nice PR but needs rebase and I think has gone stale. Ping if you'd like to pick this back up

@WillAyd WillAyd closed this Dec 17, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@WillAyd WillAyd WillAyd requested changes

@jreback jreback jreback requested changes

Assignees
No one assigned
Labels
Clean
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Ensure isinstance checks use ABC classes where available
6 participants
@kgoehner @jbrockmendel @TomAugspurger @WillAyd @jreback @kgoehner-amazon