CLN: groupby #29626
CLN: groupby #29626
Conversation
|
Hello @jbrockmendel! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found: There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻 Comment last updated at 2019-11-17 15:40:56 UTC |
pandas/core/groupby/generic.py
Outdated
| def _aggregate_named(self, func, *args, **kwargs): | ||
| result = OrderedDict() | ||
| def _aggregate_named(self, func, *args, **kwargs) -> OrderedDict: | ||
| result = OrderedDict() # type: OrderedDict |
pandas/core/groupby/generic.py
Outdated
|
|
||
| def _aggregate_named(self, func, *args, **kwargs): | ||
| result = OrderedDict() | ||
| def _aggregate_named(self, func, *args, **kwargs) -> dict: |
There was a problem hiding this comment.
Can you use typing.Dict instead of the built-in? Would also be great if you can identify key / value types if not too much incremental effort
There was a problem hiding this comment.
Will do. In cases where we can do Dict[foo, bar] i get why that's better. In cases where we can't say anything meaningful, is it just for consistency or is there some other upside?
pandas/core/groupby/generic.py
Outdated
| def _aggregate_named(self, func, *args, **kwargs): | ||
| result = OrderedDict() | ||
| def _aggregate_named(self, func, *args, **kwargs) -> Dict: | ||
| result = {} # type: Dict |
There was a problem hiding this comment.
can use the new style of typing now, e.g. result: Dict = {}, though odd that mypy can't infer that. better to be even specific, Dict[str, ]
There was a problem hiding this comment.
Just FYI mypy will always complain for an empty container creation as it can't infer the subtypes
| return result | ||
|
|
||
| def _transform_fast(self, func, func_nm) -> Series: | ||
| def _transform_fast(self, func: Callable, func_nm: str) -> Series: |
There was a problem hiding this comment.
If the arguments and/or return type of Callable can be provided it is very helpful; not sure how much incremental effort that is
pandas/core/groupby/generic.py
Outdated
| def _aggregate_named(self, func, *args, **kwargs): | ||
| result = OrderedDict() | ||
| def _aggregate_named(self, func, *args, **kwargs) -> Dict: | ||
| result = {} # type: Dict |
There was a problem hiding this comment.
Just FYI mypy will always complain for an empty container creation as it can't infer the subtypes
| result.insert(0, name, lev) | ||
|
|
||
| def _wrap_aggregated_output(self, output, names=None): | ||
| def _wrap_aggregated_output(self, output: dict, names=None): |
There was a problem hiding this comment.
dict -> typing.Dict and ideally with subtypes if possible (here and below)
|
Closing in favor of #29124 |
5 participants
splitting this off of branches that do non-CLN stuff