[REVIEW] Port scatter to libcudf++ #3354
Conversation
…to fea-port-scatter-libcudf++
Codecov Report
@@ Coverage Diff @@
## branch-0.11 #3354 +/- ##
============================================
Coverage 87.36% 87.36%
============================================
Files 49 49
Lines 9295 9295
============================================
Hits 8121 8121
Misses 1174 1174Continue to review full report at Codecov.
|
Gather does not support in-place operation, so the old approach of using gather with an inverted scatter map no longer works. Instead use thrust::scatter per-column.
…ea-port-scatter-libcudf++
…ea-port-scatter-libcudf++
|
@cwharris How would you implement repeat using scatter? |
|
@harrism I keep forgetting C++ iterators are simpler than enumerators from other languages. It would be possible if we used something more complex than a |
|
For instance, if we have a composite type contains the values we’re already iterating over and a counter, we could then make the end iterator hold the last composite value. scatter wouldn’t use the counter portion, thats just for iterator comparison. |
|
I am probably wrong about this again. I need to search through the scatter code to see if what I’m talking about makes sense. |
|
I was confusing |
|
I agree iterators are useful for scatter/gather maps. Just didn't think scatter would be useful for repeat since each input element can only have a single destination in the scatter map. Gather would work. |
Co-Authored-By: Mark Harris <[email protected]>
trevorsm7
added
5 - Ready to Merge
There was a problem hiding this comment.
I think there's ample opportunity to reduce redundancy between the scalar/column scatter code paths by updating the detail::scatter implementation to accept iterators for the inputs. Probably something where you expect the input to be a tuple<T,bool> like from make_pair_iterator.
| auto const begin = -target.num_rows(); | ||
| auto const end = target.num_rows(); | ||
| auto bounds = bounds_checker<T>{begin, end}; | ||
| CUDF_EXPECTS(thrust::all_of(rmm::exec_policy(stream)->on(stream), |
There was a problem hiding this comment.
Don't use thrust::all_of, it's slow: https://github.com/thrust/thrust/issues/1016
There was a problem hiding this comment.
Shoot, that explains why the old code I ported from was using count_if...
There was a problem hiding this comment.
I'll replace all_of with count_if in #3448
| template <typename MapIterator> | ||
| void scatter_scalar_bitmask(std::vector<std::unique_ptr<scalar>> const& source, | ||
| MapIterator scatter_map, size_type num_scatter_rows, | ||
| std::vector<std::unique_ptr<column>>& target, |
There was a problem hiding this comment.
This looks like it should be a table or mutable_table_view.
There was a problem hiding this comment.
I needed to be able to set the null count, which can't be done with a view. It could be a table. I thought it was easier to just work with the vector of columns and only move the vector into a new table as the last step. Being able to iterate over columns in the table (not just get_column(i)) would make the table a little easier to work with
| size_type const output_row = scatter_map[row]; | ||
|
|
||
| if (mark_true){ | ||
| destination.set_valid(output_row); |
There was a problem hiding this comment.
Yikes, this is going to be slow. I thought the gather_bitmask_kernel was being used?
There was a problem hiding this comment.
It's being used for the regular scatter, but it needed significant changes to make it work with the scalar variant (no source bitmask, just a single valid flag). I just ported the existing scatter scalar as-is, even though it did look like it could be improved. The changes @cwharris is making for gather_bitmask_kernel to take a selector for the source validity could fix this
|
@jrhemstad please file issues after PRs are merged or the review may get lost. |
Part of #2934, taken over from #3296
Merge after: