Fea renumbering test #1742

ChuckHastings · 2021-07-28T19:49:36Z

Adds a C++ renumbering test for integer column renumbering.

Also refactored some of the test utilities (some tech debt from the original development of the test utilities). Test graph construction now relies upon the edgelist construction calls instead of replicating functionality.

codecov-commenter · 2021-07-29T18:06:15Z

Codecov Report

Merging #1742 (4507c8c) into branch-21.10 (bf64c2c) will increase coverage by 9.72%.
The diff coverage is n/a.

@@               Coverage Diff                @@
##           branch-21.10    #1742      +/-   ##
================================================
+ Coverage         59.85%   69.57%   +9.72%     
================================================
  Files                77      139      +62     
  Lines              3547     8645    +5098     
================================================
+ Hits               2123     6015    +3892     
- Misses             1424     2630    +1206

Impacted Files	Coverage Δ
python/cugraph/community/triangle_count.py
python/cugraph/dask/components/connectivity.py
...ython/cugraph/centrality/betweenness_centrality.py
...ph/structure/graph_implementation/npartiteGraph.py
python/cugraph/centrality/katz_centrality.py
python/cugraph/dask/link_analysis/pagerank.py
python/cugraph/dask/common/read_utils.py
python/cugraph/dask/community/louvain.py
...raph/structure/graph_implementation/simpleGraph.py
python/cugraph/dask/community/__init__.py
... and 206 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 44be72a...4507c8c. Read the comment docs.

cpp/tests/experimental/renumbering_test.cpp

seunghwak · 2021-08-03T13:14:32Z

cpp/tests/experimental/renumbering_test.cpp

+        dst_v.size(),
+        renumber_map_labels_v.data(),
+        0,
+        static_cast<vertex_t>(renumber_map_labels_v.size()));


This doesn't matter in SG, but if we consider MG extension, unrenumber_local_int_vertices works only with vertices in the range of [graph_view.get_local_vertex_first(), graph_view.get_local_vertex_last()); so unrenumber_int_vertices (https://github.com/rapidsai/cugraph/blob/branch-21.10/cpp/include/cugraph/experimental/graph_functions.hpp#L212) could be more extensible.

Also note the comment in https://github.com/rapidsai/cugraph/blob/branch-21.10/cpp/include/cugraph/experimental/graph_functions.hpp#L185. If we need to unrenumber local edges in the performance critical path, we need to add addition unrenumber functions for better performance.

seunghwak · 2021-08-03T13:16:02Z

cpp/tests/utilities/test_graphs.hpp

-    // TODO: Tease through generate_graph_from_rmat_params
-    //       to extract the edgelist part
-    // Call cugraph::translate_vertex_ids(handle, d_src_v, d_dst_v, base_vertex_id_);
+    constexpr bool symmetric{true};


Do we always symmetrize R-mat graphs? This is necessary for WCC or Louvain, but there are many other algorithms that work with asymmetric R-mat graphs.

Do we always symmetrize R-mat graphs? This is necessary for WCC or Louvain, but there are many other algorithms that work with asymmetric R-mat graphs.

@ChuckHastings I'm waiting for your answer for this to approve this PR.

That was the intention of my FIXME comment. Right now the symmetrization flag is inside the test graph classes. I think the goal would be to set the symmetrization flag to false for all of the test graph classes (except file which would read it from the MM file and return the intended value) and add a test graph wrapper that will symmetrize any test graph so that Louvain and WCC could use that wrapper around rmat and other generators.

Where is the FIXME comment?

Rmat_Usecase has private member variable bool undirected_{};

So, this should be used instead of redefining symmetric here and ignoring the member variable. I assume users will be surprised if they set Rmat_Usecase.undirected_ = false and get symmetric graphs.

And it seems like construct_graph is still calling

return generate_graph_from_rmat_params<vertex_t, edge_t, weight_t, store_transposed, multi_gpu>( handle, scale_, edge_factor_, a_, b_, c_, seed_, undirected_, scramble_vertex_ids_, test_weighted, renumber, partition_ids, comm_size);

So, construct_edgelist here is actually unused. Are you planning to replace generate_graph_from_rmat_params but haven't done the job, yet? Why not replace in this PR if it's the intention?

The original implementation work here was truncated and a partially implemented capability was pushed many months ago. This is unfinished tech debt. I will look into making that change as part of this PR.

ChuckHastings · 2021-09-17T14:16:22Z

rerun tests

rlratzel · 2021-09-17T14:57:36Z

rerun tests

seunghwak

Looks good to me except for few minor complaints.

seunghwak · 2021-09-17T15:19:33Z

cpp/tests/structure/renumbering_test.cpp

+// do the perf measurements
+// enabled by command line parameter s'--perf'
+//
+static int PERF = 0;


We're no longer using PERF, this gets replaced with cugraph::test::g_perf

That's what happens when you start something and then set it aside :-)

Fixed in next push.

seunghwak · 2021-09-17T15:23:57Z

cpp/tests/structure/renumbering_test.cpp

+    }
+
+    if (PERF) {
+      handle.get_stream_view().synchronize();  // for consistent performance measurement


So, we're using CUDA_TRY(cudaDeviceSynchronize()) elsewhere.
This will be lower overhead and proper if every operation is executed on stream (this should be true... but may not be always true...) but we may better not mix the two in our tests. So, do you think we'd better use stream synchronize in our tests or we'd better continue using cudaDeviceSynchronize?

I switched to CUDA_TRY(cudaDeviceSynchronize()) in the next push.

I'm not particularly concerned about overhead in tests. We can consider that change as a general possibility later.

Yeah... me, neither. Just wasn't happy about mixing cudaDeviceSynchronize() and stream_view.synchronize(). Either is fine for me as long as we're consistent.

seunghwak · 2021-09-17T15:28:08Z

cpp/tests/structure/renumbering_test.cpp

+    ::testing::Values(cugraph::test::Rmat_Usecase(10, 16, 0.57, 0.19, 0.19, 0, false, false))));
+
+INSTANTIATE_TEST_SUITE_P(
+  rmat_large_tests,


See https://github.com/rapidsai/cugraph/blob/branch-21.10/cpp/tests/prims/mg_count_if_v.cu#L225
We renamed rmat_large_tests to rmat_benchmark_test to use this in C++ benchmarking.

Addressed in latest push.

ChuckHastings · 2021-09-18T17:13:23Z

rerun tests

ChuckHastings · 2021-09-19T02:27:59Z

@gpucibot merge

ChuckHastings added 2 commits July 27, 2021 16:09

add a C++ renumbering unit test

d9be45a

Merge branch 'branch-21.10' into fea_renumbering_test

78fb48e

ChuckHastings requested review from a team as code owners July 28, 2021 19:49

ChuckHastings self-assigned this Jul 28, 2021

ChuckHastings added 3 - Ready for Review non-breaking Non-breaking change improvement Improvement / enhancement to an existing function labels Jul 28, 2021

BradReesWork added this to the 21.10 milestone Jul 28, 2021

fix clang-format issues

6d2c9e2

seunghwak reviewed Aug 2, 2021

View reviewed changes

cpp/tests/experimental/renumbering_test.cpp Outdated Show resolved Hide resolved

remove extra line

4f2eed1

seunghwak reviewed Aug 3, 2021

View reviewed changes

BradReesWork requested a review from kaatish August 5, 2021 17:21

ChuckHastings added the 4 - Waiting on Author label Aug 11, 2021

ChuckHastings added 5 commits August 30, 2021 14:59

Merge branch 'branch-21.10' into fea_renumbering_test

41bb19d

remove experimental namespace

a23b4da

finish changes due to 21.10 merge

4921dd8

address some PR comments

b56ed56

fix clang-format issues

3c8a78a

ChuckHastings requested a review from seunghwak September 1, 2021 22:16

ChuckHastings removed the 4 - Waiting on Author label Sep 2, 2021

Merge branch 'branch-21.10' into fea_renumbering_test

d5dec1a

ChuckHastings added 4 - Waiting on Author and removed 3 - Ready for Review labels Sep 9, 2021

BradReesWork added 3 - Ready for Review and removed 4 - Waiting on Author labels Sep 10, 2021

BradReesWork requested a review from aschaffer September 10, 2021 13:29

aschaffer approved these changes Sep 10, 2021

View reviewed changes

ChuckHastings added 7 commits September 16, 2021 17:40

New construct_graph method for the unit tests

2359f21

remove old rmat_utilities file

2e19917

Merge branch 'branch-21.10' into fea_renumbering_test

3393409

update unit test added since my split from the branch

80e21eb

fix clang-format issues

8933234

delete extraneous lines

99e4713

delete extraneous lines

b43b93c

BradReesWork approved these changes Sep 17, 2021

View reviewed changes

aschaffer approved these changes Sep 17, 2021

View reviewed changes

seunghwak reviewed Sep 17, 2021

View reviewed changes

address PR comments

4507c8c

seunghwak approved these changes Sep 17, 2021

View reviewed changes

rapids-bot bot merged commit 5604446 into rapidsai:branch-21.10 Sep 19, 2021

ChuckHastings deleted the fea_renumbering_test branch February 1, 2022 16:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fea renumbering test #1742

Fea renumbering test #1742

ChuckHastings commented Jul 28, 2021 •

edited

Loading

codecov-commenter commented Jul 29, 2021 •

edited

Loading

seunghwak Aug 3, 2021

seunghwak Aug 3, 2021

seunghwak Sep 2, 2021

ChuckHastings Sep 3, 2021

seunghwak Sep 3, 2021

ChuckHastings Sep 7, 2021

ChuckHastings commented Sep 17, 2021

rlratzel commented Sep 17, 2021

seunghwak left a comment

seunghwak Sep 17, 2021

ChuckHastings Sep 17, 2021

seunghwak Sep 17, 2021

ChuckHastings Sep 17, 2021

seunghwak Sep 17, 2021

seunghwak Sep 17, 2021

ChuckHastings Sep 17, 2021

ChuckHastings commented Sep 18, 2021

ChuckHastings commented Sep 19, 2021

Fea renumbering test #1742

Fea renumbering test #1742

Conversation

ChuckHastings commented Jul 28, 2021 • edited Loading

codecov-commenter commented Jul 29, 2021 • edited Loading

Codecov Report

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

ChuckHastings commented Sep 17, 2021

rlratzel commented Sep 17, 2021

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

ChuckHastings commented Sep 18, 2021

ChuckHastings commented Sep 19, 2021

ChuckHastings commented Jul 28, 2021 •

edited

Loading

codecov-commenter commented Jul 29, 2021 •

edited

Loading