[REVIEW] Implement C/CUDA RandomWalks functionality #1439
Conversation
Merge latest release 0.17
Merge latest branch-0.18
Update forked branch-0.18
Update forked branch-0.18 from release
Update branch-0.19 from release
update forked from release branch-0.19
rlratzel
added
improvement
Codecov Report
@@ Coverage Diff @@
## branch-0.19 NVIDIA/thrust#1439 +/- ##
===============================================
- Coverage 60.72% 58.26% -2.46%
===============================================
Files 70 71 +1
Lines 3132 3283 +151
===============================================
+ Hits 1902 1913 +11
- Misses 1230 1370 +140
Continue to review full report at Codecov.
|
| EXPECT_EQ(v_ro, v_ro_expected); | ||
| EXPECT_EQ(v_ci, v_ci_expected); | ||
| EXPECT_EQ(v_vs, v_vs_expected); |
There was a problem hiding this comment.
Is the intention here to test make_graph? If so, maybe that would be better off as a separate test to 1) clean up this test, 2) make the test name more accurate, and 3) better communicate that a failure here isn't a failure in RW.
There was a problem hiding this comment.
No. The intention is to factor out make_graph() to be called in several tests to make small graphs with predictable (and observable) topology.
There was a problem hiding this comment.
I think it would be better to test make_graph() separately, then move all the repetitive inline calls to make_graph() to a SetUp().
There was a problem hiding this comment.
I'm not testing make_graph(). Is not doing enough work to warrant separate testing. It's just providing a reproducible graph to further test RW on.
make_graph() is just calling the graph_t constructor and returns a graph_t instance. This path was extensively tested when graph_t was added to our code base. Re-testing old / existing functionality doesn't bring any value, it just bloats our code base and makes tests harder to understand and isolate functionality.
…t 2 different testsuites.
… failures are blocking CI.
…ng policy to allow for seed repro testing/debugging.
…lity (start/step/stop).
| rmm::device_uvector<typename graph_t::weight_type>, | ||
| rmm::device_uvector<index_t>> | ||
| random_walks(raft::handle_t const &handle, | ||
| graph_t const &graph, |
There was a problem hiding this comment.
That seems reasonable. The louvain example (which uses graph_t - changed to graph_view_t in the latest PR) that Andrei may have copied this from does this because the same API supports both experimental and legacy graph classes. Things that only support the experimental graph classes should follow the explicit paradigm, I think.
| graph_t const &graph, | ||
| typename graph_t::vertex_type const *ptr_d_start, | ||
| index_t num_paths, | ||
| index_t max_depth); |
There was a problem hiding this comment.
num_paths is only used to convert the raw pointer/size back into a vector in your implementation. That should probably be a size_t.
Do we expect the max_depth to be a larger integer? Would hardcoding that to an int32_t be sufficient? The returned results is going to be O(num_paths * max_depth). That will limit the value that you can pass in to max_depth anyway.
…was causing 10.1 compile failures.
|
Perhaps |
BradReesWork
changed the title
Thank you! |
|
@gpucibot merge |
This PR tracks work on issue: #1380.