-
-
Notifications
You must be signed in to change notification settings - Fork 17.7k
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
API: avoid passing Manager to subclass __init__ #57553
Conversation
# this check is slightly faster, benefiting the most-common case. | ||
return df | ||
|
||
elif type(self).__name__ == "GeoDataFrame": |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Worth having a test for this in test_downstream.py
? geopandas
is installed in the CI
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good idea (also should add a test for#57032). Will wait until Joris weighs in
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
im having trouble writing a meaningful test for this cc @jorisvandenbossche
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
GeoPandas is already overriding _constructor_from_mgr
, so normally this shouldn't be needed.
(will try to take a closer look one of the coming days!)
thx @jbrockmendel |
Owee, I'm MrMeeseeks, Look at me. There seem to be a conflict, please backport manually. Here are approximate instructions:
And apply the correct labels and milestones. Congratulations — you did some good work! Hopefully your backport PR will be tested by the continuous integration and merged soon! Remember to remove the If these instructions are inaccurate, feel free to suggest an improvement. |
doc/source/whatsnew/vX.X.X.rst
file if fixing a bug or adding a new feature.Many subclasses have
__init__
methods that callsuper().__init__
, which in the relevant cases with a Manager object is deprecated. The solution in this PR is to construct the pd.DataFrame/Series and pass that to the subclass's_constructor
.This removes _sliced_from_mgr and _expanddim_from_mgr, as they are not really necessary, simplifying things a bit. Also makes _from_mgr
@final
. Can separate these simplifications into their own PR on request. xref #56681.For subclasses that currently have special handling for Manager objects in their
__init__
, they will need to move that special handling to the appropriate_constructor(_foo)?_from_mgr
method. The only package I'm aware of that does this is geopandas, so this includes a shim special-casing GeoDataFrame intended to maintain the current behavior until they have time to move that logic. cc @jorisvandenbossche ATM the shim is only in _constructor_from_mgr, pls advise on whether something analogous is needed in constructor_sliced_from_mgr and _constructor_expanddim_from_mgr.