CLN/TYP: aggregation methods in core.base

[rhshadrach]

• closes #xxxx
• tests added / passed
• passes black pandas
• passes git diff upstream/master -u -- "*.py" | flake8 --diff
• whatsnew entry

This is in order to move _aggregate and _aggregate_multiple_funcs into core.aggregation - I think that's desirable. Type-hinting is slightly more strong there (checks untyped defs), and would involve renaming self to obj. Thus the renaming of obj to selected_obj here.

[jbrockmendel]

1 mypy complaint

[rhshadrach]

Thanks @jbrockmendel. The offending line is:

name = self.name if (self.ndim == 1) else None

A natural replacement would be to do:

if isinstance(self, ABCSeries):
    name = self.name
else:
    name = None

but in general I think we prefer checking ndim == 1 rather than isinstance for performance when the object is known to be a Series/DataFrame. So the other options I see here are to use cast (is self = cast(self, ...) okay?) or just silence mypy with # type: ignore (along with the specific code).

Any advice here @simonjayhawkins, @jbrockmendel, @jreback?

[jbrockmendel]

i tend to use the ndim check, but no strong opinion

[rhshadrach]

I fixed by using cast; ready for review now.

[WillAyd]

You can just rewrite this to if isinstance(self, ABCSeries): instead of checking ndim. It would be more consistent with other checks in the code base and get rid of the need for a cast

[rhshadrach]

Agreed, at the expense of being a little slower. On my machine, I'm seeing 127 ns for ndim and cast together and 156 ns for isinstance.

[simonjayhawkins]

hmm, is that because ABCSeries resolves to Any and hence mypy is no longer checking this. So I guess a cast will be needed in the future when we turn on more strict checking options for Any.

isinstance(.., ABC..) are generally slow and should normally be avoided if possible, but here it seems the ndim check is not that much more performant.

but this is OK for now.

[WillAyd]

A 28 ns timing difference is not worth worrying about. If we wanted to instance check to be stricter we should probably just add a stub for the ABCs (or see if there's a mypy hook to strongly type them) but that is out of scope for this PR

[simonjayhawkins]

stubs for ABCs would be good.

but I see three issues with the current change.

1. impacted performance just to pass mypy. no matter how small
2. changing the typing to resolve to Any just to pass mypy. bad practice
3. code churn. no matter how trivial.

all three together and i'm now firmly -1 on this approach.

[WillAyd]

lgtm

[WillAyd]

Orthogonal to this PR but seems strange that this is really a subset of AggFuncType - any chance we have test coverage for the other parts of that Union coming through this method?

[rhshadrach]

I believe you're asking why this can't be typed Dict[Label, AggFuncType]. The last part of AggFuncType not included here are where the values are a Dict. This has been removed and will raise on 338 below. That it raises is tested in frame.apply.test_agg_dict_nested_renaming_depr.

[WillAyd]

@simonjayhawkins

[simonjayhawkins]

Thanks @rhshadrach generally lgtm.

despite my previous comment #36677 (comment) there is really no need to impact performance.

isinstance(..., ABC...) will always require a cast eventually. may as well do it now.

[rhshadrach]

@simonjayhawkins Thanks for you're input. I've changed the code back to use if ndim == 1 along with cast. I think this is what you meant by "may as well [cast] now" - let me know if I misunderstood.

[simonjayhawkins]

Thanks @rhshadrach lgtm

My objection was to ABCSeries. There is some history here #27353 and #34040

Particularly since it did NOT fix the mypy issue, just hid it, so was no better than a type: ignore

Series is already imported. So would not be against isinstance(self, Series) if that is more acceptable to others.

[jreback]

thanks @rhshadrach