Restructure Louvain to be more like other algorithms #2594

ChuckHastings · 2022-08-18T03:12:44Z

Eliminate the Louvain class and restructure the louvain algorithm to be like the other algorithms.

This will involve either inlining functionality from the Louvain member functions, or creating stand-alone detail methods that encapsulate the logic.

codecov-commenter · 2022-08-18T06:04:54Z

Codecov Report

❗ No coverage uploaded for pull request base (branch-22.10@73e66a1). Click here to learn what that means.
The diff coverage is n/a.

@@               Coverage Diff               @@
##             branch-22.10    #2594   +/-   ##
===============================================
  Coverage                ?   61.11%           
===============================================
  Files                   ?      106           
  Lines                   ?     5634           
  Branches                ?        0           
===============================================
  Hits                    ?     3443           
  Misses                  ?     2191           
  Partials                ?        0

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

ChuckHastings · 2022-08-23T19:46:58Z

cpp/src/community/detail/common_methods.cuh

@@ -0,0 +1,370 @@
+/*
+ * Copyright (c) 2020-2022, NVIDIA CORPORATION.


This file contains implementation that was formerly in louvain.cuh, so keeping the original copyright range.

seunghwak

Look much more consistent with the rest of the codebase.

I have few minor suggestions to improve code readability.

seunghwak · 2022-08-23T19:58:37Z

cpp/src/community/detail/common_methods.cuh

+  }
+};
+
+template <typename graph_view_t>


Any reason to use template <typename graph_view_t> here and template <typename vertex_t, typename edge_t, typename weight_t, bool multi_gpu> in the graph_contraction?

Verbosity more than anything.

I tend to prefer template <typename vertex_t, typename edge_t, typename weight_t, bool multi_gpu>. But the edge_src_property_t was the graph_view_type. So that would require a couple of extra lines of parameter definition, using graph_view_t is a bit clearer in that case.

I can switch to graph_view_t for detail methods for consistency if that's what we think is better.

seunghwak · 2022-08-23T20:06:15Z

cpp/src/community/detail/common_methods.cuh

+  graph_view_t const& graph_view,
+  typename graph_view_t::weight_type total_edge_weight,
+  typename graph_view_t::weight_type resolution,
+  rmm::device_uvector<typename graph_view_t::weight_type>& vertex_weights_v,


Can't this be const?

Yes, this one could be const.

seunghwak · 2022-08-23T20:11:58Z

cpp/src/community/detail/common_methods.cuh

+  typename graph_view_t::weight_type resolution,
+  rmm::device_uvector<typename graph_view_t::weight_type>& vertex_weights_v,
+  rmm::device_uvector<typename graph_view_t::vertex_type>& cluster_keys_v,
+  rmm::device_uvector<typename graph_view_t::weight_type>& cluster_weights_v,


So, this function updates clustering by delta modularity AND based on the updated clustering, updates per cluster weight sums as well. May better carve out the per cluster weight sum update part from this function?

If we remove

std::tie(cluster_keys_v, cluster_weights_v) = cugraph::transform_reduce_e_by_src_key( handle, graph_view, edge_src_dummy_property_t{}.view(), edge_dst_dummy_property_t{}.view(), graph_view_t::is_multi_gpu ? src_clusters_cache.view() : detail::edge_major_property_view_t<vertex_t, vertex_t const*>(next_clusters_v.data()), detail::return_edge_weight_t<vertex_t, weight_t>{}, weight_t{0});

cluster_keys_v and cluster_weights_v can be const as well, and it might be much easier to understand the input and output.

And if we further carve out the cache update parts, this function can take next_clusters_v as an R-value, and return the updated next_clusters_v. Then, all the input parameters will become const reference or scalars. This might be more intuitive.

I agree with @seunghwak. It would make it easier to follow then. Also in that case it would be nicer to change the function name as well.

Yes, I think I can refactor that as well. That's probably an artifact of the implementation prior to using the primitives. IIRC, I had an optimization in the original SG implementation that allowed me to skip a few of these steps if I did it all together. Clearly now they can be separated.

Refactored in next push

naimnv

The changes look to me and I will pull this branch once merged and base my work on top of it.

naimnv · 2022-08-23T20:06:58Z

cpp/tests/community/mg_louvain_test.cpp

+    ::testing::Values(cugraph::test::File_Usecase(
+      "test/datasets/karate.mtx")  //,
+                                   // cugraph::test::File_Usecase("test/datasets/dolphins.mtx")
+                      )));


Is there any particular reason for not testing with dolphin.mtx?

Nope. I had commented it out to do faster debugging on something that only failed in karate. I'll add that back in.

naimnv · 2022-08-23T20:13:45Z

cpp/src/structure/graph_view_impl.cuh

+  return transform_reduce_e(
+    handle,
+    *this,
+    edge_src_dummy_property_t{}.view(),
+    edge_dst_dummy_property_t{}.view(),
+    [] __device__(auto, auto, weight_t wt, auto, auto) { return wt; },
+    weight_t{0});
+}
+


Can we use common functor for both mg sg compute_total_edge_weight?

I'll try making a shared detail method.

Done in next push

naimnv · 2022-08-23T20:32:04Z

cpp/src/community/detail/common_methods.cuh

+    vertex_cluster_weights_v.resize(0, handle.get_stream());
+    vertex_cluster_weights_v.shrink_to_fit(handle.get_stream());
+  } else {
+    thrust::sort_by_key(handle.get_thrust_policy(),


Can we add a comment here ? Something like -
// sort cluster_keys_v_ and cluster_weights_v_ to use them for binary search (lower_bound)

Done in next push

Maybe one thing we may consider is whether should we better sort here or in compute_cluster_keys_and_values. I was at the beginning a bit confused why cluster_keys_v and cluster_weights_v are R-values.

naimnv · 2022-08-23T20:32:56Z

cpp/src/community/detail/common_methods.cuh

+                        cluster_weights_v.begin());
+
+    vertex_cluster_weights_v.resize(next_clusters_v.size(), handle.get_stream());
+    thrust::transform(handle.get_thrust_policy(),


Can we add a comment here ? Something like -

// for each cluster found in next_clusters_v_, lookup its weight in cluster_weights_v_ // and store them in local variable vertex_cluster_weights_v

Done in next push

naimnv · 2022-08-23T20:36:46Z

cpp/src/community/detail/common_methods.cuh

+  typename graph_view_t::weight_type resolution,
+  rmm::device_uvector<typename graph_view_t::weight_type>& vertex_weights_v,
+  rmm::device_uvector<typename graph_view_t::vertex_type>& cluster_keys_v,
+  rmm::device_uvector<typename graph_view_t::weight_type>& cluster_weights_v,


I agree with @seunghwak. It would make it easier to follow then. Also in that case it would be nicer to change the function name as well.

seunghwak

LGTM

ChuckHastings · 2022-08-26T17:32:41Z

@gpucibot merge

first cut at louvain restructuring

85549c4

ChuckHastings self-assigned this Aug 18, 2022

ChuckHastings added 2 - In Progress improvement Improvement / enhancement to an existing function non-breaking Non-breaking change labels Aug 18, 2022

ChuckHastings added this to the 22.10 milestone Aug 18, 2022

ChuckHastings added 3 commits August 18, 2022 18:02

more tweaks to louvain

6eda1a1

more cleanup, prepare for louvain_impl to become an hpp file

479ab28

Merge branch 'branch-22.10' into create_louvain_detail_methods

0edd870

ChuckHastings commented Aug 23, 2022

View reviewed changes

ChuckHastings added 3 - Ready for Review and removed 2 - In Progress labels Aug 23, 2022

ChuckHastings marked this pull request as ready for review August 23, 2022 19:48

ChuckHastings requested review from a team as code owners August 23, 2022 19:48

ChuckHastings changed the title ~~[WIP] Restructure Louvain to be more like other algorithms~~ Restructure Louvain to be more like other algorithms Aug 23, 2022

ChuckHastings requested review from seunghwak and naimnv August 23, 2022 19:49

fix clang-format issues

944080b

seunghwak reviewed Aug 23, 2022

View reviewed changes

naimnv approved these changes Aug 23, 2022

View reviewed changes

ChuckHastings added 2 commits August 25, 2022 10:38

address PR comments

55d78d4

fix clang-format issues

44befe7

seunghwak approved these changes Aug 25, 2022

View reviewed changes

naimnv approved these changes Aug 26, 2022

View reviewed changes

rapids-bot bot merged commit 421bac0 into rapidsai:branch-22.10 Aug 26, 2022

ChuckHastings deleted the create_louvain_detail_methods branch December 2, 2022 18:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Restructure Louvain to be more like other algorithms #2594

Restructure Louvain to be more like other algorithms #2594

ChuckHastings commented Aug 18, 2022

codecov-commenter commented Aug 18, 2022 •

edited

Loading

ChuckHastings Aug 23, 2022

seunghwak left a comment

seunghwak Aug 23, 2022

ChuckHastings Aug 23, 2022

seunghwak Aug 23, 2022

ChuckHastings Aug 23, 2022

seunghwak Aug 23, 2022

naimnv Aug 23, 2022

ChuckHastings Aug 23, 2022

ChuckHastings Aug 24, 2022

naimnv left a comment

naimnv Aug 23, 2022

ChuckHastings Aug 23, 2022

naimnv Aug 23, 2022 •

edited

Loading

ChuckHastings Aug 23, 2022

ChuckHastings Aug 24, 2022

naimnv Aug 23, 2022 •

edited

Loading

ChuckHastings Aug 24, 2022

seunghwak Aug 25, 2022

naimnv Aug 23, 2022 •

edited

Loading

ChuckHastings Aug 24, 2022

naimnv Aug 23, 2022

seunghwak left a comment

ChuckHastings commented Aug 26, 2022

		@@ -0,0 +1,370 @@
		/*
		* Copyright (c) 2020-2022, NVIDIA CORPORATION.

Restructure Louvain to be more like other algorithms #2594

Restructure Louvain to be more like other algorithms #2594

Conversation

ChuckHastings commented Aug 18, 2022

codecov-commenter commented Aug 18, 2022 • edited Loading

Codecov Report

Choose a reason for hiding this comment

seunghwak left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

naimnv left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

naimnv Aug 23, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

naimnv Aug 23, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

naimnv Aug 23, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seunghwak left a comment

Choose a reason for hiding this comment

ChuckHastings commented Aug 26, 2022

codecov-commenter commented Aug 18, 2022 •

edited

Loading

naimnv Aug 23, 2022 •

edited

Loading

naimnv Aug 23, 2022 •

edited

Loading

naimnv Aug 23, 2022 •

edited

Loading