Rename primitive functions. #2234

seunghwak · 2022-04-19T17:42:26Z

Partially address #2003
Rename

copy_v_transform_reduce_in|out_nbr => transform_reduce_incoming|outgoing_e_of_v
copy_v_transform_reduce_key_aggregated_out_nbr => transform_reduce_key_aggregated_outgoing_e_of_v
update_frontier_v_push_if_out_nbr => update_frontier_from_outgoing_e_of_v

…renaming update

…rim_names

ChuckHastings · 2022-04-26T15:01:54Z

rerun tests

…rim_names

kaatish · 2022-04-26T15:37:45Z

cpp/include/cugraph/graph_view.hpp

+ * vertex_partition_range_offsets[row_comm_size * i + row_comm_rank] and b_i =
+ * vertex_partition_range_offsets[row_comm_size * i + row_comm_rank + 1]. c is
+ * vertex_partition_range_offsets[row_comm_size * col_comm_rank] and d =
+ * vertex_partition_range_offsets[row_comm_size * (col_comm_rank + 1)].


Could we rename this to specify that it is a range of vertex ids?
vertex_id_ranges perhaps?

To be more consistent with the rest, we may need to rename vertex_partition_range to vertex_id_partition_range but I feel like this is a bit too lengthy (I thought vertex_partition somewhat implies partitioning of vertex ID ranges but it seems like this implication is not clear enough).

I added additional comments below to make this point clearer. Do you think this is sufficient or we need to further discuss about renaming?

Yeah, this explanation is clear enough.

kaatish · 2022-04-26T15:38:54Z

cpp/include/cugraph/prims/transform_reduce_incoming_outgoing_e_of_v.cuh

-                                 EdgeOp e_op,
-                                 T init,
-                                 VertexValueOutputIterator vertex_value_output_first)
+void transform_reduce_e_of_v(raft::handle_t const& handle,


We could just use transform_reduce_edges here.

See

#2234 (comment)

kaatish · 2022-04-26T15:39:57Z

cpp/include/cugraph/prims/transform_reduce_incoming_outgoing_e_of_v.cuh

@@ -985,7 +982,7 @@ template <typename GraphViewType,
          typename EdgeOp,
          typename T,
          typename VertexValueOutputIterator>
-void copy_v_transform_reduce_out_nbr(
+void transform_reduce_outgoing_e_of_v(


transform_reduce_outgoing_edges?
The of_v part seems unnecessary to me.

Oh, this is for per vertex reduction (previously copy_v_transform_reduce_in|out_nbr).

I thought this two level naming scheme is confusing,

So, for global reduction, we use _e or _v

and per vertex reduction, we use incoming/outgoing_e_of_v

Besides this clarification, do you have other suggestions?

I would read transform_reduce_outgoing_e_of_v as 'transform reduce outgoing edges of vertices'.
The 'of vertices' doesn't really communicate that a per source reduction is being done. We should think of something that does make this clear.

We could name it exactly as transform_reduce_e to denote that every edge is being iterated on. The fact that this variant is doing per source reduction could be explained by differing function signature. This could be confusing to some users.

We could also use transform_reduce_outgoing_edges_per_source or '..._e_per_src'.

Another point was that if we are talking in terms of edges, in my opinion we should use the term source/destination (src/dst) instead of vertex.
Perhaps I am overlooking some nuances when dealing with transposed (csc) graphs.

Agreed, I like the per_src idea. I will make this change.

Will using src nomenclature still make sense even when the graph is transposed?

I assume so.

But transform_reduce_outgoing_e_of_v and transform_reduce_e_per_src may have slightly different mental models.

I initially hoped outgoing_e_of_v to be interpreted as "for every vertex, iterate over outgoing edges and reduce the edge functor outputs per vertex"; so this is more vertex centric.

"e_per_src" might be interpreted as iterate over every directed edge, apply a functor to each edge, than reduce functor outputs per source vertex ID; so this is more edge centric.

Both are pretty much doing the same thing, but nuances might be slightly different, and I feel like the latter is a bit clearer.

Whether the graph is transposed or not affects efficiency of the primitive implementation. Per source reduction can be more efficient with a source-major format while per destination reduction can be more efficient with a destination-major format (by avoiding atomics, and some primitives allow only a source-major format... this is more to avoid implementing something that will not be used in the foreseeable future).

My main concern about per_src nomenclature is about 0-(in|out)-degree vertices.

So these primitives are designed to serve properties_of_v = some_initial_value + sum_over_incoming|outgoing_e (calculated per edge value) (e.g. PageRank, Katz centrality, HITS, summing per vertex incoming/outgoing edge weights).

This doesn't align very well with the edge-centric mental model.

kaatish · 2022-04-26T15:40:26Z

cpp/include/cugraph/prims/transform_reduce_key_aggregated_outgoing_e_of_v.cuh

@@ -204,7 +202,7 @@ template <typename GraphViewType,
          typename ReduceOp,
          typename T,
          typename VertexValueOutputIterator>
-void copy_v_transform_reduce_key_aggregated_out_nbr(
+void transform_reduce_key_aggregated_outgoing_e_of_v(


transform_reduce_key_aggregated_outgoing_edges?

See

#2234 (comment)

kaatish · 2022-04-26T15:41:00Z

cpp/src/link_analysis/hits_impl.cuh

@@ -105,7 +105,7 @@ std::tuple<result_t, size_t> hits(raft::handle_t const& handle,
  }
  for (size_t iter = 0; iter < max_iterations; ++iter) {
    // Update current destination authorities property
-    copy_v_transform_reduce_in_nbr(
+    transform_reduce_incoming_e_of_v(


transform_reduce_incoming_edges?

See

#2234 (comment)

kaatish · 2022-04-26T15:47:03Z

cpp/src/traversal/bfs_impl.cuh

@@ -222,7 +222,7 @@ void bfs(raft::handle_t const& handle,
        e_op.prev_visited_flags = prev_visited_flags.data();
      }

-      update_frontier_v_push_if_out_nbr(
+      update_frontier_from_outgoing_e_of_v(


If we are using the sources/destinations nomenclature then this could be renamed to be
update_frontier_from_destinations because the frontier will only contain vertices that are destinations as opposed to sources.
Having said that update_frontier_from_outgoing_edges is fine too.

What about update_frontier_from_outgoing_e to be more consistent with the rest? (we are using e to denote edges so I hope this abbreviation is not too confusing).

I initially used e_of_v instead of just e borrowing this convention from other primitives performing per vertex reduction, but here, we are not performing per vertex reduction. Starting from an input frontier, iterate over all the outgoing edges from the vertices in the frontier, then, compute a new frontier by filtering out destination vertices of the outgoing edges.

Something like compute_new_frontier_from_outgoing_e_of_v_in_input_frontier might be more descriptive (but too verbose) and I assume update_frontier part reasonably covers both compute_new_frontier and of_v_in_input_frontier.

What about update_frontier_from_outgoing_e to be more consistent with the rest?

Yes, this works

seunghwak · 2022-04-26T21:07:35Z

@ChuckHastings @kaatish Added "do not merge" label.

Still needs to figure out how to properly re-name copy_v_transform_reduce_in|out_nbr (or other two level primitives).

transform_reduce_incoming|outgoing_e_of_v may not be interpreted as per vertex reduction.
transform_reduce_e_per_src|dst may have an issue with 0 in|out-degree vertices.

I'm now thinking more about for_each_v_transform_reduce_incoming|outgoing_e... but still thinking about better names.

…rim_names

codecov-commenter · 2022-04-28T01:04:57Z

Codecov Report

Merging #2234 (ae1de41) into branch-22.06 (38be932) will increase coverage by 0.48%.
The diff coverage is n/a.

❗ Current head ae1de41 differs from pull request most recent head 161f634. Consider uploading reports for the commit 161f634 to get more accurate results

@@               Coverage Diff                @@
##           branch-22.06    #2234      +/-   ##
================================================
+ Coverage         70.82%   71.30%   +0.48%     
================================================
  Files               170      171       +1     
  Lines             11036    11169     +133     
================================================
+ Hits               7816     7964     +148     
+ Misses             3220     3205      -15

Impacted Files	Coverage Δ
python/cugraph/cugraph/centrality/__init__.py	`100.00% <ø> (ø)`
...thon/cugraph/cugraph/centrality/katz_centrality.py	`88.23% <ø> (-1.24%)`	⬇️
python/cugraph/cugraph/dask/common/part_utils.py	`19.23% <ø> (-0.94%)`	⬇️
python/cugraph/cugraph/gnn/graph_store.py	`82.60% <ø> (+11.77%)`	⬆️
python/cugraph/cugraph/structure/hypergraph.py	`90.00% <ø> (ø)`
python/cugraph/cugraph/tests/dask/test_mg_bfs.py	`0.00% <ø> (ø)`
python/cugraph/cugraph/tests/dask/test_mg_comms.py	`0.00% <ø> (ø)`
...cugraph/cugraph/tests/dask/test_mg_connectivity.py	`0.00% <ø> (ø)`
...ython/cugraph/cugraph/tests/dask/test_mg_degree.py	`0.00% <ø> (ø)`
python/cugraph/cugraph/tests/dask/test_mg_hits.py	`0.00% <ø> (ø)`
... and 27 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 2956d56...161f634. Read the comment docs.

ChuckHastings · 2022-04-28T01:43:05Z

@gpucibot merge

rename two level primitive files

cc2404e

seunghwak added 2 - In Progress improvement Improvement / enhancement to an existing function breaking Breaking change labels Apr 19, 2022

seunghwak added this to the 22.06 milestone Apr 19, 2022

seunghwak requested a review from a team as a code owner April 19, 2022 17:42

seunghwak self-assigned this Apr 19, 2022

seunghwak added 3 commits April 19, 2022 14:18

rename primitive function calls

be5884c

rename test files

14c1945

rename prim function calls in tests

1685445

seunghwak changed the title ~~[WIP][skip-ci] Rename primitive functions.~~ [WIP] Rename primitive functions. Apr 19, 2022

seunghwak added 2 commits April 19, 2022 14:39

update CMake

3ec334e

additional variable renamings

56aab7c

seunghwak requested a review from a team as a code owner April 19, 2022 21:41

fix errors in print-outs

9f5a9a3

seunghwak changed the title ~~[WIP] Rename primitive functions.~~ Rename primitive functions. Apr 19, 2022

seunghwak added 3 - Ready for Review and removed 2 - In Progress labels Apr 19, 2022

copyright year

63c6d26

seunghwak requested review from kaatish and ChuckHastings April 19, 2022 21:51

seunghwak added 5 commits April 19, 2022 16:42

piggyback vertex_partition_offsets to vertex_partition_range_offsets …

86233f2

…renaming update

Merge branch 'branch-22.06' of github.com:rapidsai/cugraph into enh_p…

a3e2104

…rim_names

Merge branch 'branch-22.06' of github.com:rapidsai/cugraph into enh_p…

463b75d

…rim_names

Merge branch 'branch-22.06' of github.com:rapidsai/cugraph into enh_p…

2a93c7f

…rim_names

fix compile error

cf5406d

ChuckHastings approved these changes Apr 26, 2022

View reviewed changes

Merge branch 'branch-22.06' of github.com:rapidsai/cugraph into enh_p…

2e019f2

…rim_names

kaatish reviewed Apr 26, 2022

View reviewed changes

seunghwak added 5 commits April 26, 2022 09:16

update comments about partition_t

b3a1e32

rename update_frontier... file

1e80fb7

remove of_v from update_frontier... prim name

f40e3ab

rename update_frontier... test flie name

ac62ee2

rename prim files

9711389

kaatish approved these changes Apr 26, 2022

View reviewed changes

seunghwak added the DO NOT MERGE Hold off on merging; see PR for details label Apr 26, 2022

seunghwak added 4 commits April 27, 2022 13:17

renmae prim related files

04e37e0

code update based on primitive renamings

3aca5be

Merge branch 'branch-22.06' of github.com:rapidsai/cugraph into enh_p…

d001193

…rim_names

variable renaming in tests

161f634

seunghwak removed the DO NOT MERGE Hold off on merging; see PR for details label Apr 27, 2022

rapids-bot bot merged commit c8caf7c into rapidsai:branch-22.06 Apr 28, 2022

seunghwak deleted the enh_prim_names branch August 11, 2022 23:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rename primitive functions. #2234

Rename primitive functions. #2234

seunghwak commented Apr 19, 2022 •

edited

Loading

ChuckHastings commented Apr 26, 2022

kaatish Apr 26, 2022

seunghwak Apr 26, 2022 •

edited

Loading

kaatish Apr 26, 2022

kaatish Apr 26, 2022

seunghwak Apr 26, 2022

kaatish Apr 26, 2022

seunghwak Apr 26, 2022

kaatish Apr 26, 2022

kaatish Apr 26, 2022 •

edited

Loading

seunghwak Apr 26, 2022

kaatish Apr 26, 2022 •

edited

Loading

seunghwak Apr 26, 2022

seunghwak Apr 26, 2022

seunghwak Apr 26, 2022

kaatish Apr 26, 2022

seunghwak Apr 26, 2022

kaatish Apr 26, 2022

seunghwak Apr 26, 2022

kaatish Apr 26, 2022

seunghwak Apr 26, 2022

kaatish Apr 26, 2022

seunghwak commented Apr 26, 2022

codecov-commenter commented Apr 28, 2022 •

edited

Loading

ChuckHastings commented Apr 28, 2022

Rename primitive functions. #2234

Rename primitive functions. #2234

Conversation

seunghwak commented Apr 19, 2022 • edited Loading

ChuckHastings commented Apr 26, 2022

Choose a reason for hiding this comment

seunghwak Apr 26, 2022 • edited Loading

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

kaatish Apr 26, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kaatish Apr 26, 2022 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seunghwak commented Apr 26, 2022

codecov-commenter commented Apr 28, 2022 • edited Loading

Codecov Report

ChuckHastings commented Apr 28, 2022

seunghwak commented Apr 19, 2022 •

edited

Loading

seunghwak Apr 26, 2022 •

edited

Loading

kaatish Apr 26, 2022 •

edited

Loading

kaatish Apr 26, 2022 •

edited

Loading

codecov-commenter commented Apr 28, 2022 •

edited

Loading