SubClassedDataFrame.groupby().mean() etc. use method of SubClassedDataFrame

[AdamOrmondroyd]

• closes BUG: DataFrameSubClass.groupby() doesn't use methods of subclass #51757
• Tests added and passed if fixing a bug or adding a new feature
• All code checks passed.
• Added type annotations to new arguments/methods/functions.
• Added an entry in the latest doc/source/whatsnew/vX.X.X.rst file if fixing a bug or adding a new feature.

I appreciate that changing the behaviour for all subclasses is overkill, so pointers on how to better do this would be great

[jbrockmendel]

this is trouble bc in general we can't assume that _constructor is a class

[AdamOrmondroyd]

What else could it be (practically speaking, I know it's Callable)?

[jbrockmendel]

Geopandas has a callable that can dispatch to different classes. @jorisvandenbossche has argued against deprecating allowing this.

[AdamOrmondroyd]

I've added a check whether self._obj_1d_constructor is a Series

[jorisvandenbossche]

I appreciate that changing the behaviour for all subclasses is overkill

I think that this might indeed be an issue for subclasses that don't override those methods. If we want to keep a fix like this, maybe checking whether the method is the one from pandas or not might be better (something like self.obj.mean is Series.mean instead of type(self.obj) is Series).

Because with the current change, if you have a subclass that doesn't influence how aggregations are done, you would get a big slowdown because of the current PR I think?

[jorisvandenbossche]

Could this also be something like lambda group, **kwargs: group.value_counts(**kwargs)?

(didn't look in detail at how this code is working, so potentially this doesn't make sense at all)

[AdamOrmondroyd]

Is the WeightedDataFrame thing referenced in #51757 the motivating case here?

Yes, our classes add a set of "weights" to the index which are used to calculate statistics such as the mean.

What if instead of this we did did something like

class DataFrame:
    @property
    def _gb_cls(self):
        return DataFrameGroupBy

and then had DataFrame.groupby return an instance of _gb_cls. Then subclasses could override that.

That would function, though feel this would be unexpected extra work for developers, so should be mentioned in the extending pandas section of the docs.

I was very surprised that groupby() doesn't just use the methods of its underlying class. If DataFrame.mean() and Series.mean() are themselves optimised, what is gained by GroupBy reimplementing them?

[jbrockmendel]

If DataFrame.mean() and Series.mean() are themselves optimised, what is gained by GroupBy reimplementing them?

Performance. The paths that iterate over groups generally make a sorted copy of the DataFrame to make that iteration faster, but the iteration itself is still costly.

[AdamOrmondroyd]

If DataFrame.mean() and Series.mean() are themselves optimised, what is gained by GroupBy reimplementing them?

Performance. The paths that iterate over groups generally make a sorted copy of the DataFrame to make that iteration faster, but the iteration itself is still costly.

but, if these behaviours only change for subclasses, then performance is only lost for subclasses which override mean etc, in which case surely it is preferable that the intended results are returned rather than a fast one?

[jbrockmendel]

but, if these behaviours only change for subclasses, then performance is only lost for subclasses which override mean etc, in which case surely it is preferable that the intended results are returned rather than a fast one?

The question was why we use a cython path instead of iterating over DataFrame.mean. I gave an explanation of the status quo, not an argument against the fix suggested here.

[AdamOrmondroyd]

but, if these behaviours only change for subclasses, then performance is only lost for subclasses which override mean etc, in which case surely it is preferable that the intended results are returned rather than a fast one?

The question was why we use a cython path instead of iterating over DataFrame.mean. I gave an explanation of the status quo, not an argument against the fix suggested here.

Apologies, I misunderstood the difference you were highlighting.

How do you think I should proceed?

[jbrockmendel]

i think issubclass rather than isinstance?

[jbrockmendel]

these will be wrong if _constructor is not a class

[jbrockmendel]

How do you think I should proceed?

Unless there is a viable way to handle your use case with an ExtensionArray (i.e. the weights would be somehow included with the values, not at the DataFrame level), then the approach here seems fine (albeit ugly)

[jbrockmendel]

i think this belongs below in the Groupby/resample/rolling section?

[jbrockmendel]

if this pattern is going to show up a lot, does it merit a decorator?

[AdamOrmondroyd]

I've had a first pass at making a decorator, not sure how to deal with engine and engine_kwargs

[mroeschke]

Thanks for the pull request, but it appears to have gone stale. If interested in continuing, please merge in the main branch, address any review comments and/or failing tests, and we can reopen.