Transform Reduce E test#1798
Conversation
kaatish
added
2 - In Progress
non-breaking
Templating fixes in thrust tuple utils
kaatish
added
bug
kaatish
added
3 - Ready for Review
and removed
2 - In Progress
bug
Codecov Report
@@ Coverage Diff @@
## branch-21.10 #1798 +/- ##
================================================
+ Coverage 59.85% 69.58% +9.73%
================================================
Files 77 137 +60
Lines 3547 8581 +5034
================================================
+ Hits 2123 5971 +3848
- Misses 1424 2610 +1186 Continue to review full report at Codecov.
|
seunghwak
left a comment
seunghwak
left a comment
There was a problem hiding this comment.
I really like the updates you made to dataframe_buffer.cuh!!!
It might be better to update the test code to be readable by users without strong expertise in template metaprogramming (don't go too far if it requires significantly sacrificing code quality... and I understand it might not be possible... but just in case if possible). I am just a bit worried about scaring potential graph primitives users who are not very familiar with TMP.
This PR looks really good otherwise.
| e_op_result_sum = | ||
| block_reduce_edge_op_result<e_op_result_t, transform_reduce_e_for_all_block_size>().compute( | ||
| e_op_result_sum); | ||
| e_op_result_sum = BlockReduce(temp_storage).Reduce(e_op_result_sum, edge_property_add); |
There was a problem hiding this comment.
As we discussed, if block_reduce_edge_op_result can serve what we need, we'd better delete this and directly use cub everywhere for consistency.
There was a problem hiding this comment.
block_reduce_edge_op_result is still being used by copy_v_transform_reduce_nbr and therefore cannot be removed yet. It should be removed though because it is buggy. Perhaps in another PR?
There was a problem hiding this comment.
Yeah... you can remove it when you write test cases for copy_v_transform_reduce_nbr.
seunghwak
left a comment
seunghwak
left a comment
There was a problem hiding this comment.
This looks great and thanks for the updates!!!
| e_op_result_sum = | ||
| block_reduce_edge_op_result<e_op_result_t, transform_reduce_e_for_all_block_size>().compute( | ||
| e_op_result_sum); | ||
| e_op_result_sum = BlockReduce(temp_storage).Reduce(e_op_result_sum, edge_property_add); |
There was a problem hiding this comment.
Yeah... you can remove it when you write test cases for copy_v_transform_reduce_nbr.
|
@gpucibot merge |
5 participants
This PR adds multi-gpu testing for transform_reduce_e primitive.
A bug was fixed in transform_reduce_e where shared memory was being allocated inside a device function for the purpose of using it with cub::BlockReduce.
Utilities in dataframe_buffer.cuh were cleaned up. dataframe_buffer related functions with the exception of allocation functions no longer require an explicit template parameter.