Several improvements to zip_iterator/zip_function #1710
Merged
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
bernhardmgruber
commented
May 6, 2024
bernhardmgruber
commented
May 6, 2024
ahendriksen
reviewed
May 6, 2024
| _CCCL_HOST_DEVICE Function& underlying_function() const | ||
| { | ||
| return func; | ||
| } |
There was a problem hiding this comment.
Thanks for adding this. This makes it easier to "undo" the zip_function when the zip iterator has been destructured, and will be really helpful for the thrust::transform performance work.
miscco
reviewed
May 6, 2024
thrust/thrust/zip_function.h
Outdated
| @@ -154,6 +140,9 @@ template <typename Function> | |||
| class zip_function | |||
| { | |||
| public: | |||
| //! Default constructs Function. | |||
There was a problem hiding this comment.
Please Ignore: I am not a fan of comments that add no information
Suggested change
| //! Default constructs Function. | |
| //! Default constructs function. |
There was a problem hiding this comment.
I hate that too, so what do you prefer:
- Drop the comment
- Reword to "Default constructs the contained function object"
I thought I would make it clear that there is no partially formed state, and you can call a default constructed zip_function.
Also, I spelled it Function on purpose, because it is the name of the class template parameter. function does not refer to any entity within this scope.
There was a problem hiding this comment.
"Default constructs the contained function object." it is.
🟨 CI Results [ Failed: 115 | Passed: 83 | Total: 198 ]
|
| # | Runner |
|---|---|
| 154 | linux-amd64-cpu16 |
| 16 | linux-arm64-cpu16 |
| 16 | linux-amd64-gpu-v100-latest-1 |
| 12 | windows-amd64-cpu16 |
👃 Inspect Changes
Modifications in project?
| Project | |
|---|---|
| CCCL Infrastructure | |
| libcu++ | |
| CUB | |
| +/- | Thrust |
Modifications in project or dependencies?
| Project | |
|---|---|
| CCCL Infrastructure | |
| libcu++ | |
| +/- | CUB |
| +/- | Thrust |
miscco
approved these changes
May 7, 2024
🟩 CI Results [ Failed: 0 | Passed: 198 | Total: 198 ]
|
| # | Runner |
|---|---|
| 154 | linux-amd64-cpu16 |
| 16 | linux-arm64-cpu16 |
| 16 | linux-amd64-gpu-v100-latest-1 |
| 12 | windows-amd64-cpu16 |
👃 Inspect Changes
Modifications in project?
| Project | |
|---|---|
| CCCL Infrastructure | |
| libcu++ | |
| CUB | |
| +/- | Thrust |
Modifications in project or dependencies?
| Project | |
|---|---|
| CCCL Infrastructure | |
| libcu++ | |
| +/- | CUB |
| +/- | Thrust |
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.
This PR adds a few improvements to
zip_iterator/zip_function, which come out of the collaboration with @ahendriksen who is playing with thrust in BabelStream. There, we need to unwrap zip iterators/functions, which is why I added a few such observers. I also modernized and improved the documentation examples a bit.