BUG: Fix Grouper with a datetime key in conjunction with another key

[juleek]

• closes BUG: pd.Grouper with a datetime key in conjunction with another key generates incorrect number of group keys. #51158
• 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.

Description of the bug

The crux of the issue was in BaseGrouper::groups:

to_groupby = zip(*(ping.grouping_vector for ping in self.groupings))

We are going to iterate over to_groupby, which is a zip of ping.grouping_vector.

groupings are populated in get_grouper, below is an excerpt of relevant code that we executed in case of grouping by date:

        # Taking this branch because grouping_vector is `TimeGrouper`
        elif isinstance(grouping_vector, Grouper):
            newgrouper, newobj = grouping_vector._get_grouper(self.obj, validate=False)
            # newgrouper is BinGrouper after the line above
            if isinstance(newgrouper, ops.BinGrouper):
                grouping_vector = newgrouper

after this, Grouping::grouping_vector is BinGrouper, and this is what zip() will try to iterate over.

Before the fix, when we were iterating over BinGrouper, its base class __iter__ was used, which was using self.indices, which was:

defaultdict(<class 'list'>, {Timestamp('2000-01-01 00:00:00'): [0, 1, 2], 
                             Timestamp('2000-01-02 00:00:00'): [3, 4, 5]})

When you iterate over a dictionary in Python, you iterate over the keys, and more importantly we have only two of them here. Second Grouping has array in its grouping vector consisting of 6 elements:

['a' 'b' 'a' 'b' 'a' 'b']

so, the zip() function just truncated it.

Fix

By adding __iter__ in BinGrouper, the zip() function now iterate over the elements from self.groupings[0].grouping_vector, which is, in our case:

DatetimeIndex(['2000-01-01', '2000-01-01', '2000-01-01', '2000-01-02',
               '2000-01-02', '2000-01-02'],
              dtype='datetime64[ns]', name='b', freq=None)

[juleek]

@jbrockmendel @rhshadrach Can you please have a look and/or advise? Thanks.

[jbrockmendel]

why is the parent class method wrong?

[juleek]

Sorry, I forgot to add description. Please, see the description of the PR.

[rhshadrach]

I wonder if it would be better to refactor the indices here to align better with the base class rather than override iter. But I'm not certain if that's a good idea and I'm okay with this fix. cc @jbrockmendel

[rhshadrach]

@jbrockmendel - friendly ping.

[jbrockmendel]

Sorry for the delay. i had in mind something more like @rhshadrach's suggestion, that needing this here might indicate that the self.indices (which the parent class method iterates over) might be wrong

[juleek]

@jbrockmendel I fixed all your comments. Can you please review the PR? Thanks.

[juleek]

@jbrockmendel I am wondering whether you had time to have a look the PR? If not, should I add someone else to the PR to review it?

Also, am I missing something or the tests a quite flaky?
For example, right now NumPy is failing with this:

Fatal Python error: Segmentation fault

[jbrockmendel]

Please be patient, there are a lot of PRs to review.

The segfault is unrelated and is affecting all PRs at the moment. You can ignore it.

[juleek]

@jbrockmendel have you had time to have a look?

[juleek]

@jbrockmendel Any news, please?

[rhshadrach]

lgtm; I've opened #52468 to track a possible followup.

[rhshadrach]

Thanks @juleek!