CLN: type up core.groupby.grouper.get_grouper #29458

topper-123 · 2019-11-07T07:33:28Z

Add types + make some minor cleanups.

WillAyd

Cool thanks for doing this

WillAyd · 2019-11-07T16:00:52Z

pandas/core/groupby/grouper.py

@@ -438,9 +438,9 @@ def _get_grouper(
    observed=False,
    mutated=False,
    validate=True,
-):
+) -> Tuple[BaseGrouper, List[Hashable], NDFrame]:


Instead of NDFrame should use FrameOrSeries which will ensure this function is generic; should also change obj a few lines up from NDFrame to FrameOrSeries` (unless this does for whatever reason interchange between Series and DataFrame objects)

Are you sure FrameOrSeries works as intended? I can't get it to work in PyCharm, so I opted to use NDFrame, so I could get the type hints from NDFrame. at least.

I'm not clear on the error you see in PyCharm but for sure we use it elsewhere. You would probably need to change the annotation for obj in this func

I've changed it to use FrameOrSeries

WillAyd · 2019-11-07T16:01:40Z

pandas/core/groupby/grouper.py

@@ -517,7 +517,7 @@ def _get_grouper(
        if key.key is None:
            return grouper, [], obj
        else:
-            return grouper, {key.key}, obj
+            return grouper, [key.key], obj


So is there no impact changing this from a set to a list? If not, does it even need to be a container?

This just makes the types consistent for all returns. A set or a list is not important when there's only 1 item in the container.

BTW It sometimes has lists with several items, so needs to be a list/container.

Hmm not sure I follow. Maybe better to ask what the error was that mypy was giving? I assume this gets mutated outside of the function right?

Oh, it wasn't a mypy error, I think the function should have a clear return type, so List rather than Contaioner.

(More general): I think function may have broad parameter types, but should strive to have clear return types, so there's less ambiguity over what you're looking at. Don't you agree?

OK. Yea I'm not necessarily saying the old container type was correct and this is wrong, I guess was just questioning why this even needs to be held in a container if it's simply one value being returned.

Can be refactored or reviewed in a follow up if that's a big effort; not a blocker as is

Yeah, I'm haven't checked if a single value could be returned, instead of the list.

My motivation for doing this is mostly wanting to pin down what I'm looking at in groupBy, Grouper etc. I think "real" clean-ups must come later. It's quite complex to me now, and I'm hoping these typeups will ease understanding.

Sounds good. Yea this part of code is tricky and could use some refactor, so this is all moving in the right direction

WillAyd · 2019-11-07T16:03:31Z

pandas/core/groupby/grouper.py

@@ -573,7 +571,7 @@ def _get_grouper(
            all_in_columns_index = all(
                g in obj.columns or g in obj.index.names for g in keys
            )
-        elif isinstance(obj, Series):
+        else:  # Series


Hmm is there a reason to remove this isinstance check? Seems to loosen the type checker

I had the reverse problem where no else clause made the type unknown. An alternative would be to do:

elif isinstance(obj, Series): .... else: raise TypeError(...)

?

the next line accesses obj.index, so implicitly pins it down. the comment above is fine or an assertion if that helps mypy out

Not sure I read you comment clearly. The chosen solution os ok for you ( if not preferred:-))?

I think assert isinstance(obj, Series) within the branch should work then. Sounds like it doesn't make much of a difference with what is hitting this path now, but the original intent is definitely only for Series objects to make it down this path

not checked, but I assume that mypy can't perform type narrowing between a DataFrame and Series since we use NDFrame as the bound for FrameOrSeries.

so we know that if obj is not a DataFrame then it must be a Series, but mypy can't know that.

if it's a public method, and we don't have control of the calling types then #29458 (comment) makes sense

jbrockmendel · 2019-11-07T17:41:32Z

pandas/core/groupby/grouper.py

@@ -3,7 +3,7 @@
 split-apply-combine paradigm.
 """

-from typing import Optional, Tuple
+from typing import Hashable, List, Optional, Tuple


there are some places in these modules where name attrs are annotated as str or Optional[str] where Hashable would be more accurate I think

WillAyd · 2019-11-07T19:58:55Z

pandas/core/groupby/grouper.py

@@ -573,7 +571,7 @@ def _get_grouper(
            all_in_columns_index = all(
                g in obj.columns or g in obj.index.names for g in keys
            )
-        elif isinstance(obj, Series):
+        else:  # Series


I think assert isinstance(obj, Series) within the branch should work then. Sounds like it doesn't make much of a difference with what is hitting this path now, but the original intent is definitely only for Series objects to make it down this path

WillAyd

lgtm @simonjayhawkins

simonjayhawkins · 2019-11-08T08:55:06Z

Thanks @topper-123

topper-123 added Clean Typing type annotations, mypy/pyright type checking labels Nov 7, 2019

topper-123 added this to the 1.0 milestone Nov 7, 2019

topper-123 force-pushed the clean_get_grouper branch 3 times, most recently from a6f53cf to 18f1516 Compare November 7, 2019 15:53

WillAyd requested changes Nov 7, 2019

View reviewed changes

jbrockmendel reviewed Nov 7, 2019

View reviewed changes

WillAyd requested changes Nov 7, 2019

View reviewed changes

topper-123 added 2 commits November 7, 2019 22:04

type up core.groupby.grouper.get_grouper

34bbbd7

change NDFrame to FrameOrSeries

015a223

topper-123 force-pushed the clean_get_grouper branch from 44f70c8 to 015a223 Compare November 7, 2019 22:09

WillAyd approved these changes Nov 8, 2019

View reviewed changes

simonjayhawkins merged commit 67ee16a into pandas-dev:master Nov 8, 2019

simonjayhawkins mentioned this pull request Nov 8, 2019

TYPING: change to FrameOrSeries Alias in pandas._typing #29480

Closed

topper-123 deleted the clean_get_grouper branch November 8, 2019 14:58

Reksbril pushed a commit to Reksbril/pandas that referenced this pull request Nov 18, 2019

CLN: type up core.groupby.grouper.get_grouper (pandas-dev#29458)

1db7264

proost pushed a commit to proost/pandas that referenced this pull request Dec 19, 2019

CLN: type up core.groupby.grouper.get_grouper (pandas-dev#29458)

247dd71

proost pushed a commit to proost/pandas that referenced this pull request Dec 19, 2019

CLN: type up core.groupby.grouper.get_grouper (pandas-dev#29458)

5c1d615

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CLN: type up core.groupby.grouper.get_grouper #29458

CLN: type up core.groupby.grouper.get_grouper #29458

topper-123 commented Nov 7, 2019

WillAyd left a comment

WillAyd Nov 7, 2019

topper-123 Nov 7, 2019

WillAyd Nov 7, 2019

topper-123 Nov 7, 2019

WillAyd Nov 7, 2019

topper-123 Nov 7, 2019

topper-123 Nov 7, 2019

WillAyd Nov 7, 2019

topper-123 Nov 7, 2019

WillAyd Nov 7, 2019

topper-123 Nov 7, 2019

WillAyd Nov 7, 2019

WillAyd Nov 7, 2019

topper-123 Nov 7, 2019

jbrockmendel Nov 7, 2019

topper-123 Nov 7, 2019

WillAyd Nov 7, 2019

simonjayhawkins Nov 8, 2019

jbrockmendel Nov 7, 2019

WillAyd Nov 7, 2019

WillAyd left a comment

simonjayhawkins commented Nov 8, 2019

CLN: type up core.groupby.grouper.get_grouper #29458

CLN: type up core.groupby.grouper.get_grouper #29458

Conversation

topper-123 commented Nov 7, 2019

WillAyd left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

WillAyd left a comment

Choose a reason for hiding this comment

simonjayhawkins commented Nov 8, 2019