-
Notifications
You must be signed in to change notification settings - Fork 653
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
FIX-#5091: Handle pd.Grouper objects correctly #6174
Conversation
Signed-off-by: Karthik Velayutham <[email protected]>
@@ -573,11 +573,19 @@ def _internal_by(self): | |||
internal_by = tuple() | |||
if self._drop: | |||
if is_list_like(self._by): | |||
internal_by = tuple(by for by in self._by if isinstance(by, str)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if you're fixing the order here, can you port the test case from #6164 and address the comments I left there?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
resolved. there's a separate test case for order now, but this doesn't fix #6164
@@ -573,11 +573,19 @@ def _internal_by(self): | |||
internal_by = tuple() | |||
if self._drop: | |||
if is_list_like(self._by): | |||
internal_by = tuple(by for by in self._by if isinstance(by, str)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
resolved. there's a separate test case for order now, but this doesn't fix #6164
) | ||
def test_groupby_with_grouper(by): | ||
# See https://github.com/modin-project/modin/issues/5091 for more details | ||
data = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@pyrito can you add some more columns too? We should have multiple column partitions on ray/dask
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
@pyrito you now have a merge conflict 😢 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks @pyrito !
…oject#6174) Signed-off-by: Karthik Velayutham <[email protected]>
What do these changes do?
This PR addresses two issues - it handles pd.Grouper objects correctly and also obeys the order of the groupby sorting keys as passed in.
flake8 modin/ asv_bench/benchmarks scripts/doc_checker.py
black --check modin/ asv_bench/benchmarks scripts/doc_checker.py
git commit -s
docs/development/architecture.rst
is up-to-date