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

Stable sorting order for -0.0 and +0.0 for float and double. #218

Open
wants to merge 8 commits into
base: main
from

Conversation

@canonizer
Copy link
Collaborator

@canonizer canonizer commented Oct 17, 2020

This modifies digit extraction in radix sorting to sort -0.0 and +0.0 in stable order.

@allisonvacanti allisonvacanti added this to the 1.11.1 milestone Oct 19, 2020
@allisonvacanti
Copy link
Collaborator

@allisonvacanti allisonvacanti commented Oct 21, 2020

@canonizer Have you looked into at the performance impact of this? If not, I can use this as a case study for my new benchmarking code, but that will probably mean this won't make the 1.11.0 release.

@allisonvacanti allisonvacanti self-assigned this Oct 27, 2020
@allisonvacanti allisonvacanti added this to Inbox in Current Milestone via automation Oct 27, 2020
@allisonvacanti allisonvacanti modified the milestones: 1.11.1, 1.11.0 Oct 27, 2020
@allisonvacanti
Copy link
Collaborator

@allisonvacanti allisonvacanti commented Oct 27, 2020

This implementation looks good to me, and the performance difference is negligible (<1% difference on GV100/GA100 from @canonizer's benchmarks).

Though now I'm curious whether this is actually expected behavior for users, or if it's better to just let them pre-condition their data when they want these to be treated equivalently. I'll do some polls and confirm whether or not this is something folks expect.

@allisonvacanti
Copy link
Collaborator

@allisonvacanti allisonvacanti commented Oct 27, 2020

Looks like the stl already does stable sorts this way, so that's a nice, definitive answer :)

@canonizer Can you update the DeviceRadixSort documentation to specify this behavior, and add a regression test to test/test_device_radix_sort.cu? I'll take a look at the other backends in Thrust to see if we can provide this guarantee there, too.

@canonizer
Copy link
Collaborator Author

@canonizer canonizer commented Oct 27, 2020

I will update the documentation and add the test.

@allisonvacanti allisonvacanti moved this from Inbox to Draft [PRs] in Current Milestone Oct 30, 2020
@allisonvacanti
Copy link
Collaborator

@allisonvacanti allisonvacanti commented Nov 11, 2020

@canonizer Any updates on this? I'd need to land this by Friday to make the 1.11.0 release, otherwise we can slip to 1.12.0.

cub/block/block_radix_sort.cuh Outdated Show resolved Hide resolved
cub/block/block_radix_sort.cuh Outdated Show resolved Hide resolved
cub/block/block_radix_sort.cuh Outdated Show resolved Hide resolved
cub/device/device_radix_sort.cuh Outdated Show resolved Hide resolved
cub/device/device_segmented_radix_sort.cuh Outdated Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Current Milestone
  
Draft [PRs]
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants
You can’t perform that action at this time.