TYP: resolve mypy ignores in core/apply.py #47066
Conversation
| def normalize_keyword_aggregation(kwargs: dict) -> tuple[dict, list[str], list[int]]: | ||
| def normalize_keyword_aggregation( | ||
| kwargs: dict, | ||
| ) -> tuple[dict, list[str], npt.NDArray[np.intp]]: |
There was a problem hiding this comment.
npt.NDArray[intp] is the actual retrun type of Index.get_indexer
| @@ -1116,7 +1117,8 @@ def apply_empty_result(self) -> Series: | |||
| ) | |||
|
|
|||
| def apply_standard(self) -> DataFrame | Series: | |||
| f = self.f | |||
| # caller is responsible for ensuring that f is Callable | |||
| f = cast(Callable, self.f) | |||
There was a problem hiding this comment.
This is the same approach as taken for the other cases (for example, apply_str).
I think we might be able avoid the casts altogether by passing self.f as an argument to the apply_X functions, if that would be a desirable change.
There was a problem hiding this comment.
I assume self.f : Callable | None? I think it would be nicer to assert f is not None
There was a problem hiding this comment.
self.f is typed as AggFuncType (defined here), which does not include None. Do we still have to check in that case?
There was a problem hiding this comment.
For context, type checking is done in this method.
Re-reading the code, though, it seems that this method actually does not handle the AggFuncTypeDict case. I'll change the input signature.
IMO this kind of error highlights the reasons for avoiding casts altogether - would it be acceptable to pass self.f as an argument to the specific apply_ functions, so that we can propagate the inferred types?
There was a problem hiding this comment.
Sounds reasonable to me, might be good to check git blame and then cc the person who last touched this method.
There was a problem hiding this comment.
that would be @rhshadrach 🙂
There was a problem hiding this comment.
Within the init of the Apply class, self.f is packed with args/kwargs when appropriate. This keeps that logic DRY; would passing f instead mean that logic needs to be repeated multiple places? If so, I'd be opposed to the change.
Another option would be to not pack self.f with args/kwargs at all, and only do so in the appropriate circumstances later in the code. That may be a better option altogether, but I'm not sure if there are other issues that might arise if one attempts this.
There was a problem hiding this comment.
I think delaying the arg packing of self.f might be an interesting option, since it looks like this is only used when f is Callable (so the apply_default case). But it seems like that might be a bigger change that affects more code, so I think it's better to explore that in a separate PR.
pandas/core/apply.py
Outdated
| @@ -1049,7 +1050,7 @@ class SeriesApply(NDFrameApply): | |||
| def __init__( | |||
| self, | |||
| obj: Series, | |||
| func: AggFuncType, | |||
| func: SeriesAggFuncType, | |||
There was a problem hiding this comment.
func can be a dict here, no?
There was a problem hiding this comment.
Apologies, you are right - I did not realize dict was considered to be list-like. I reverted that part.
xref #37715