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

Add the DeviceSelect::FlaggedIf algorithm #1533

Merged
merged 8 commits into from
Mar 27, 2024
Merged

Conversation

gonidelis
Copy link
Member

@gonidelis gonidelis commented Mar 13, 2024

Description

This adds the cub::DeviceSelect::FlaggedIf algorithm which combines the predicate selection approach of cub::DeviceSelect::If and the flag approach of cub::DeviceSelect::Flagged.

This fixes #1409.

Checklist

  • New or existing tests cover these changes.
  • The documentation is up to date with these changes.

Copy link
Collaborator

@elstehle elstehle left a comment

Choose a reason for hiding this comment

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

Thank you for picking this up!

Overall. this looks good already. I think we want to add documentation and examples for the newly introduced interfaces and extend test coverage a bit more.

cub/cub/device/device_select.cuh Outdated Show resolved Hide resolved
@gonidelis
Copy link
Member Author

Thank you for picking this up!

Overall. this looks good already. I think we want to add documentation and examples for the newly introduced interfaces and extend test coverage a bit more.

Working on the tests rn - that's why I submitted it as a draft. Well, on the documentation too... :|

@gonidelis gonidelis marked this pull request as ready for review March 20, 2024 19:42
@gonidelis gonidelis requested review from a team as code owners March 20, 2024 19:42
@gonidelis gonidelis requested a review from elstehle March 22, 2024 00:55
Copy link
Collaborator

@elstehle elstehle left a comment

Choose a reason for hiding this comment

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

Thanks a lot for the great work! Looks good, just a couple of minor suggestions.

cub/test/catch2_test_device_select_api.cu Outdated Show resolved Hide resolved
cub/test/catch2_test_device_select_api.cu Show resolved Hide resolved
cub/test/catch2_test_device_select_api.cu Outdated Show resolved Hide resolved
cub/test/catch2_test_device_select_api.cu Show resolved Hide resolved
cub/test/catch2_test_device_select_api.cu Outdated Show resolved Hide resolved
cub/cub/device/device_select.cuh Show resolved Hide resolved
cub/test/catch2_test_device_select_flagged_if.cu Outdated Show resolved Hide resolved
cub/test/catch2_test_device_select_flagged_if.cu Outdated Show resolved Hide resolved
cub/test/catch2_test_device_select_flagged_if.cu Outdated Show resolved Hide resolved
cub/test/catch2_test_device_select_flagged_if.cu Outdated Show resolved Hide resolved
@gonidelis
Copy link
Member Author

gonidelis commented Mar 22, 2024

@elstehle I fixed everything except the get_reference() portion of it in the unit test, which I want to isolate in a separate commit. Thanks for the feedback!

ps: Need to also extract the in-place API example in an separate executable test.

@gonidelis gonidelis force-pushed the stencilif branch 2 times, most recently from c096964 to 47f3a50 Compare March 22, 2024 23:47
Copy link
Collaborator

@gevtushenko gevtushenko left a comment

Choose a reason for hiding this comment

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

Looks good! Just to make sure that in-place overload compiles, I'd like to see its snippet extracted into API test.

cub/cub/device/device_select.cuh Outdated Show resolved Hide resolved
cub/test/catch2_test_device_select_api.cu Outdated Show resolved Hide resolved
Copy link
Collaborator

@elstehle elstehle left a comment

Choose a reason for hiding this comment

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

Just two minor nitpicks remaining, otherwise good to go 🚀

cub/test/catch2_test_device_select_flagged_if.cu Outdated Show resolved Hide resolved
@gonidelis gonidelis requested a review from elstehle March 26, 2024 20:48
@gonidelis gonidelis merged commit 20a4866 into NVIDIA:main Mar 27, 2024
585 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

[FEA]: Expose new StencilIf functionality in cub::DeviceSelect
3 participants