Test select_random_vertices for all possible values of flags #4042
Conversation
…vertex partition range size on each GPU
naimnv
added
improvement
…vertex partition range size on each GPU
| @@ -903,6 +903,9 @@ weight_t compute_total_edge_weight( | |||
| * @param select_count The number of vertices to select from the graph | |||
| * @param with_replacement If true, select with replacement, if false select without replacement | |||
| * @param sort_vertices If true, return the sorted vertices (in the ascending order). | |||
| * @param shuffle_int_to_local If true and If @p given_set is not specified | |||
| * then shuffle internal (i.e. renumbered) vertices to their local GPUs based on vertex | |||
| * partitioning, otherwise shuffle as many vertices as local vertex partition size to each GPU. | |||
There was a problem hiding this comment.
Why should we care about renumbering here? And why apply this flag only when @p given_set is not specified?
There was a problem hiding this comment.
I think I have to change all of these.
Objective: For a graph with N vertices, we want to generate N unique random numbers in range [0, N) (across GPUs) and we don't want to shuffle the generated numbers to GPUs based on vertex partitioning, and each GPU gets graph_view.local_vertex_partition_range_size() many random numbers,
while keeping supporting the existing behavior of select_random_vertices.
There was a problem hiding this comment.
Does it make sense now @seunghwak ?
There was a problem hiding this comment.
I guess I'm wondering why this even needs to be part of the select_random_vertices call.
While there is randomness to the feature that we need, I think all that we need is a "distributed shuffle" function. Given an rmm::device_uvector<T> on each GPU that is initialized with some values, we want the equivalent of thrust::shuffle of that distributed device vector.
I'm not aware of an algorithm that would need to both select a random subset of the vertex list and randomly order that list across all of the GPUs. If there was such an algorithm it could first call select_random_vertices and then call a new randomly shuffle a distributed device vector function.
I don't see a benefit of conflating the two actions into a single function.
There was a problem hiding this comment.
Changes havebeen be aborted. Shuffling will be done by the ECG implementation itself.
There was a problem hiding this comment.
If this PR is not meant to be merged, should we better close this PR?
There was a problem hiding this comment.
Actually all the changes to the select_random_vertices has been aborted, but the test has been updated. Please review the tests.
…vertex partition range size on each GPU
…vertex partition range size on each GPU
naimnv
changed the title
|
/ok to test |
|
/ok to test |
|
/ok to test |
|
/merge |
Test
select_random_verticesfor all possible values of flags.