Fix valid count computation in offset_bitmask_binop kernel #13489
Conversation
This file contains 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
davidwendt
added
bug
davidwendt
requested review from
bdice,
mythrocks and
vyasr
and removed request for
a team
vyasr
approved these changes
Jun 1, 2023
There was a problem hiding this comment.
Thanks for finding this @davidwendt! Also great work getting repros @wence- @shwina.
Approving with one minor suggestion for improvement.
bdice
reviewed
Jun 1, 2023
bdice
approved these changes
Jun 1, 2023
bdice
reviewed
Jun 1, 2023
bdice
reviewed
Jun 1, 2023
bdice
approved these changes
Jun 1, 2023
There was a problem hiding this comment.
New changes look great. One commented line can be deleted. Otherwise let's get this fix merged! Very nice work @davidwendt. 💯
|
Just to note this latest version is still good on our reproducers for me. |
wence-
approved these changes
Jun 2, 2023
3 tasks
rapids-bot bot
pushed a commit
that referenced
this pull request
Jun 9, 2023
Fixes some logic in the `groupby/sort/sort_helper.cu` to improve memoization safety. Some functions cache their results so they may be called several times. This change ensures the cached variable is fully formed before returning it so that parallel calls do not get a partially built result. The code also simplifies some of the logic by replacing some thrust calls with equivalent `cudf::detail` function calls that return columns. Found while working on #13489 Authors: - David Wendt (https://github.com/davidwendt) Approvers: - Yunsong Wang (https://github.com/PointKernel) - Nghia Truong (https://github.com/ttnghia) URL: #13521
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.
Description
Fixes the valid count calculation in the
offset_bitmask_binopkernel when the mask may contain slack bits. The slack bits must be accounted for in the final bitmask word to correctly compute the valid count.Closes #13479
Checklist