enforced return types of groupby sequence arguments #10271
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
By distinguishing between scalar vs. sequence arguments
we can now correctly return a scalar/tuple of the corresponding
group values that is grouped over.
This makes autonomous usage of `groupby` more predictable.
E.g.
().groupby("x")[0] is a scalar
whereas
().groupby(["x"])[0] is a tuple of length 1
This meant some slight restructuring in the
core.groupby code, but it should be relatively simple.
Signed-off-by: Nick Papior <nickpapior@gmail.com>
Signed-off-by: Nick Papior <nickpapior@gmail.com>
|
@dcherian some pointers here would be helpful, I don't fully understand the implications here... :( |
dcherian
reviewed
Apr 29, 2025
| @@ -429,6 +430,8 @@ def _parse_group_and_groupers( | |||
| ) | |||
| for group, grouper in grouper_mapping.items() | |||
| ) | |||
| if isinstance(group, str): | |||
There was a problem hiding this comment.
instead I'd return a flag squeeze_group_label_on_iter that is True if group is str
dcherian
reviewed
Apr 29, 2025
| @@ -626,7 +628,7 @@ class GroupBy(Generic[T_Xarray]): | |||
| def __init__( | |||
| self, | |||
| obj: T_Xarray, | |||
| groupers: tuple[ResolvedGrouper, ...], | |||
| groupers: ResolvedGrouper | tuple[ResolvedGrouper, ...], | |||
| restore_coord_dims: bool = True, | |||
There was a problem hiding this comment.
Suggested change
| restore_coord_dims: bool = True, | |
| squeeze_group_label_on_iter: bool | |
| restore_coord_dims: bool = True, |
There was a problem hiding this comment.
and then I'd handle this inside __iter__ where you'll need
for label, group in zip(self.unique_coord.data, self._iter_grouped(), strict=True):
# handle promotion to tuple
yield label, group
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
By distinguishing between scalar vs. sequence arguments we can now correctly return a scalar/tuple of the corresponding group values that is grouped over.
This makes autonomous usage of
groupbymore predictable.E.g.
().groupby("x")[0] is a scalar
whereas
().groupby(["x"])[0] is a tuple of length 1
This meant some slight restructuring in the
core.groupby code, but it should be relatively simple.
whats-new.rstapi.rst