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.

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

BUG: remove usage of _constructor._get_axis_number (fix when _constructor properties return callables) #46018

Merged

Conversation

jorisvandenbossche
Copy link
Member

@jorisvandenbossche jorisvandenbossche commented Feb 16, 2022

See report at #32860 (comment) (and we also run into this one case in geopandas). Also related to #32638

As far as I know this is the only remaining case where we rely on _constructor/_constructor_sliced/_constructor_expanddim returning a class and not a generic callable (in geopandas, we also have a workaround for this one case).

This specific case was introduced in #34837 to avoid circular dependencies/imports, so therefore I added the DataFrame import inline.

@jorisvandenbossche jorisvandenbossche added the Subclassing Subclassing pandas objects label Feb 16, 2022
@jorisvandenbossche
Copy link
Member Author

Found one more, in replace() which was introduced in #39152 (restored the inline import that was there before that PR)

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.

no objection, can you add a whatsnew

@jreback
Copy link
Contributor

jreback commented Feb 16, 2022

tests are failing as well :->

@jorisvandenbossche
Copy link
Member Author

Ah, that's another case, but where we didn't call a method on it, but used in an issubclass check.

@@ -539,7 +542,7 @@ def __init__(
self.new_axes = self._get_new_axes()

def get_result(self):
cons: type[DataFrame | Series]
cons: Callable
Copy link
Member

Choose a reason for hiding this comment

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

could annotate as Callable returning DataFrame|Series

Copy link
Member Author

Choose a reason for hiding this comment

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

The Callable typing is actually giving errors .. (with or without return type)

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, but I probably have to update the actual properties' annotation to callable as well

@jorisvandenbossche
Copy link
Member Author

Are people OK with considering this for 1.4.x? (the actual change is quite small, the bulk of the diff is due to typing)

This is actually fixing a regression for geopandas, although not a regression in 1.4.0, but somewhere between 1.0 and 1.4.

@jorisvandenbossche jorisvandenbossche added this to the 1.4.2 milestone Feb 24, 2022
@jreback jreback merged commit 19aa5ab into pandas-dev:main Feb 26, 2022
@jreback
Copy link
Contributor

jreback commented Feb 26, 2022

@meeseeksdev backport 1.4.x

@lumberbot-app

This comment was marked as resolved.

@lumberbot-app

This comment was marked as resolved.

@jreback
Copy link
Contributor

jreback commented Feb 26, 2022

@jorisvandenbossche if you can push a manual backport

@simonjayhawkins
Copy link
Member

The auto backport failed as #45272 is not backported. The changes here to that code were only the mypy comments. Those changes have therefore been omitted in the manual backport.

simonjayhawkins added a commit that referenced this pull request Mar 4, 2022
…r (fix when _constructor properties return callables) (#46228)

Co-authored-by: Joris Van den Bossche <[email protected]>
yehoshuadimarsky pushed a commit to yehoshuadimarsky/pandas that referenced this pull request Jul 13, 2022
@jorisvandenbossche jorisvandenbossche deleted the subclass-constructor_expanddim branch July 17, 2022 09:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Subclassing Subclassing pandas objects
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants