Add optional-iterator support to indexalator #9306
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
davidwendt
added
2 - In Progress
Codecov Report
@@ Coverage Diff @@
## branch-21.12 #9306 +/- ##
================================================
- Coverage 10.79% 10.77% -0.02%
================================================
Files 116 116
Lines 18869 19390 +521
================================================
+ Hits 2036 2090 +54
- Misses 16833 17300 +467
Continue to review full report at Codecov.
|
davidwendt
added
3 - Ready for Review
vuule
approved these changes
Sep 27, 2021
ttnghia
reviewed
Sep 30, 2021
ttnghia
reviewed
Sep 30, 2021
ttnghia
approved these changes
Sep 30, 2021
There was a problem hiding this comment.
Just one more thing I'm a bit concerned but may not be able to address for now: The tests calls this->iterator_test_thrust(expected_values, it_dev, host_values.size());, instead of using the macro EXPECT_COLUMNS_EQUAL etc. If a test fails, we may have difficulty locating which line it is failing.
|
@gpucibot merge |
rapids-bot bot
pushed a commit
that referenced
this pull request
Oct 13, 2021
This PR changes the `cudf::detail::copy_if_else` utility functions to accept an optional-iterator instead of a pair-iterator. This improves the compile time of source files by generating 4x less kernels since the two input data arrays can each have nulls requiring 4 different pair-iterators to be created to call it. The optional-iterator allows the nulls check to occur at runtime instead of compile time. The changes in this PR are for the callers of `detail::copy_if_else` to provide optional-iterators instead of pair-iterators. This PR is dependent on the changes in #9306 The benchmarks for the effected calling functions showed no significant change in runtime performance using the single optional-iterator over 4 unique pair-iterators. Two additional benchmarks are included cover non-null measurement which this PR impacts the most. Also, the `copy_tests.cu` was renamed `copy_tests.cpp` and the test that launched to the internal `cudf::detail::copy_if_else_kernel` was replaced with one with a data-set large enough to require multiple blocks. Related changes for the strings specific `cudf::strings::detail::copy_if_else` are in #9266 Authors: - David Wendt (https://github.com/davidwendt) Approvers: - Bradley Dice (https://github.com/bdice) - Robert Maynard (https://github.com/robertmaynard) - Jake Hemstad (https://github.com/jrhemstad) URL: #9324
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 support for the
optional-iteratorinterface to thecudf::detail::indexalator. This will allow replacing usages ofpair-iteratorin future PR(s). This also includes gtests that were missing for theindexalator.