Skip to content
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

Implement sort_remaining for sort_index #14033

Merged

Conversation

wence-
Copy link
Contributor

@wence- wence- commented Sep 4, 2023

Description

Previously, the sort_remaining argument to sort_index was ignored. Passing sort_remaining=False would raise a NotImplementedError. Moreover, for a multiindex, sort_remaining=True was not handled correctly: if not all levels were requested as sorted, sort_index would behave as if sort_remaining=False had been passed.

To fix this case, construct the sort order based on first the provided levels and, if sort_remaining=True, the left-over levels (in index order).

To facilitate this, refactor the internal _get_columns_by_label function to always return a Frame-like object (previously, if we had a Frame we would get back a ColumnAccessor, and it was only for IndexedFrame and above that we'd get something of Self-like type back). This meant that calling _get_sorted_inds with by != None was not possible on an Index or MultiIndex (the code assumed we'd get a Frame back).

Checklist

  • I am familiar with the Contributing Guidelines.
  • New or existing tests cover these changes.
  • The documentation is up to date with these changes.

@wence- wence- requested a review from a team as a code owner September 4, 2023 14:50
@wence- wence- requested review from vyasr and mroeschke September 4, 2023 14:50
@github-actions github-actions bot added the Python Affects Python cuDF API. label Sep 4, 2023
@wence- wence- added bug Something isn't working non-breaking Non-breaking change and removed Python Affects Python cuDF API. labels Sep 4, 2023
Copy link
Contributor

@galipremsagar galipremsagar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, minor comment.

python/cudf/cudf/tests/test_multiindex.py Show resolved Hide resolved
@wence-
Copy link
Contributor Author

wence- commented Sep 5, 2023

/merge

@rapids-bot rapids-bot bot merged commit 0b01fe4 into rapidsai:branch-23.10 Sep 5, 2023
55 checks passed
@wence- wence- deleted the wence/fea/sort-index-sort-remaining branch September 6, 2023 10:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working non-breaking Non-breaking change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[FEA] Allow DataFrame.sort_index(level=n) to tiebreak on lexsortedness in mode.pandas_compatable
4 participants