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

Fix loc-getitem ordering when index contains duplicate labels #13659

Merged
merged 5 commits into from
Jul 12, 2023

Conversation

wence-
Copy link
Contributor

@wence- wence- commented Jul 4, 2023

Description

If the index has duplicate labels then it is not sufficient to sort with the ordering induced by gathering the requested keys since the duplicate keys arrive in non-deterministic order from the join in the table that is being looked up from. To fix this, sort additionally by a secondary key that is the ordering induced by gathering from the donor table.

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 July 4, 2023 09:51
@wence- wence- requested review from shwina and isVoid July 4, 2023 09:51
@github-actions github-actions bot added the Python Affects Python cuDF API. label Jul 4, 2023
If the index has duplicate labels then it is not sufficient to sort
with the ordering induced by gathering the requested keys since the
duplicate keys arrive in non-deterministic order from the join in the
table that is being looked up from. To fix this, sort additionally by
a secondary key that is the ordering induced by gathering from the
donor table.

- Closes rapidsai#13658
@wence- wence- force-pushed the wence/fix/issue-13658 branch from e82fb11 to 65bf861 Compare July 6, 2023 10:26
@wence- wence- added bug Something isn't working non-breaking Non-breaking change labels Jul 6, 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.

Overall LGTM, minor comment

python/cudf/cudf/core/dataframe.py Outdated Show resolved Hide resolved
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.

Fixed up some style issues, now looks good to go!

@galipremsagar galipremsagar added the 5 - Ready to Merge Testing and reviews complete, ready to merge label Jul 12, 2023
@galipremsagar
Copy link
Contributor

/merge

@wence-
Copy link
Contributor Author

wence- commented Jul 12, 2023

Thanks Prem.

@rapids-bot rapids-bot bot merged commit 241257f into rapidsai:branch-23.08 Jul 12, 2023
@wence- wence- deleted the wence/fix/issue-13658 branch October 24, 2024 09:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
5 - Ready to Merge Testing and reviews complete, ready to merge bug Something isn't working non-breaking Non-breaking change Python Affects Python cuDF API.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] loc-based lookup where index has repeated labels does not (always) match pandas order
2 participants