Add new primitives: compute_in|out_degrees, compute_in|out_weight_sums to graph_view_t #1394
Conversation
|
@ChuckHastings FYI, https://github.com/rapidsai/cugraph/blob/branch-0.18/cpp/src/experimental/louvain.cuh#L556 should eventually be replaced with graph_view_t's compute_out_weight_sums(). I am not touching this at this moment assuming you're actively modifying the Louvain code. |
BradReesWork
added
improvement
|
rerun tests |
seunghwak
changed the title
|
rerun tests |
Codecov Report
@@ Coverage Diff @@
## branch-0.19 #1394 +/- ##
==============================================
Coverage ? 60.75%
==============================================
Files ? 70
Lines ? 3134
Branches ? 0
==============================================
Hits ? 1904
Misses ? 1230
Partials ? 0 Continue to review full report at Codecov.
|
| compute_out_degrees(raft::handle_t const& handle) const | ||
| { | ||
| if (store_transposed) { | ||
| return compute_minor_degrees(handle, *this); |
There was a problem hiding this comment.
I noticed we have started to use minor/major terminology but seeing this here used concurrently with src/dst and in/out terminology makes me wonder if that's really helping. We should try to stick to the smallest possible set of names that refer to the same thing.
Reconciling this is beyond this PR, we should have a separate issue/PR for this.
There was a problem hiding this comment.
So the origin of minor/major might be from https://github.com/rapidsai/cugraph/blob/branch-0.19/cpp/include/patterns/copy_v_transform_reduce_in_out_nbr.cuh and this terminology has been used since the start of graph primitives.
This was introduced mainly for source code reuse, noting that iterating over incoming edges when the graph adjacency matrix is stored as is (CSR) and iterating over outgoing edges when the graph adjacency matrix stored as transposed (CSC) can be implemented using the same code.
Yeah... we may open a several issue and we may need to go through more intense discussion on terminologies as these are now used all over the graph primitives. If there is a clear benefit to make a switch, yes we should make a switch, but I assume there should be a very strong justification.
There was a problem hiding this comment.
And this might be a much bigger discussion, but we may later more intensively investigate the performance benefit of storing graph adjacency matrix as transposed.
This was mainly introduced to avoid atomics, but AFAIK, GPU's performance on atomics has improved drastically in the past some years and if the performance gain is not large enough, we may reconsider this issue. If the performance gain is marginal, dropping this will significantly simplify our codebase.
There was a problem hiding this comment.
And this terminology issue will become irrelevant.
There was a problem hiding this comment.
Sounds good to me. Thanks for elaborating. Let's open an issue to keep track of this.
BradReesWork
changed the title
|
@gpucibot merge |
…1423) Implement the `update_by_delta_modularity` method using the new graph primitives and pattern accelerators. This eliminates all of the custom MNMG implementation originally created for MNMG Louvain a few releases ago and replaces it with the new pattern accelerator and graph primitives that have been added in the last couple of releases. This depends on the following PRs and should not be merged until after them: * #1394 * #1399 closes #1220 Authors: - Chuck Hastings (@ChuckHastings) Approvers: - Andrei Schaffer (@aschaffer) - Seunghwa Kang (@seunghwak) - Rick Ratzel (@rlratzel) - Alex Fender (@afender) URL: #1423
Close #1208