Move edge triangle count to the stable API #4382
Conversation
alexbarghi-nv
added
improvement
…6_stable-edge_triangle-count
… into branch-24.06_stable-edge_triangle-count
| #include <vector> | ||
|
|
||
| struct EdgeTriangleCount_Usecase { | ||
| bool test_weighted_{false}; |
There was a problem hiding this comment.
Need to include edge_masking.
There was a problem hiding this comment.
And we need mg_edge_triangle_count_test as well (is this code MNMG ready?)
There was a problem hiding this comment.
Need to include edge_masking.
I included edge mask tests
is this code MNMG ready?
Yes it is
| std::array<bool, 2>{true, true}, | ||
| false /*FIXME: pass 'do_expensive_check' as argument*/); | ||
| size_t approx_edges_to_intersect_per_iteration = | ||
| static_cast<size_t>(handle.get_device_properties().multiProcessorCount) * (1 << 17); |
There was a problem hiding this comment.
This isn't approximate, right? (I mean except for the last chunk). This approx_ naming convention is used when we are dealing with the CSR data structure and # edges should need to be aligned with the CSR offset array boundaries.
There was a problem hiding this comment.
I mean except for the last chunk
Ya or if it is bigger than the number of edges. But I agree it doesn't align with the intended context (CSR data structure). I renamed it to edges_to_intersect_per_iteration
| std::optional<edge_property_view_t<edge_t, weight_t const*>>{std::nullopt}, | ||
| std::optional<edge_property_view_t<edge_t, edge_t const*>>{std::nullopt}, | ||
| std::optional<raft::device_span<vertex_t const>>(std::nullopt)); | ||
|
|
||
| auto edge_first = thrust::make_zip_iterator(edgelist_srcs.begin(), edgelist_dsts.begin()); | ||
|
|
||
| thrust::sort(handle.get_thrust_policy(), edge_first, edge_first + edgelist_srcs.size()); |
There was a problem hiding this comment.
What's the purpose of sort here?
There was a problem hiding this comment.
Good catch. We no longer need this as we do not pass an edgelist to edge_triangle_count. In fact when we decompress the edges from the graph, they are already sorted.
| ? (edgelist_srcs.size() / approx_edges_to_intersect_per_iteration) | ||
| : (edgelist_srcs.size() / approx_edges_to_intersect_per_iteration) + 1; | ||
| size_t prev_chunk_size = 0; | ||
| auto num_edges = edgelist_srcs.size(); |
There was a problem hiding this comment.
num_remaining_edges to be clearer?
There was a problem hiding this comment.
And prev_chunk_size = edgelist_srcs.size() - num_remaining_edges, so redundant.
There was a problem hiding this comment.
num_remaining_edges to be clearer?
Done
And prev_chunk_size = edgelist_srcs.size() - num_remaining_edges, so redundant
Can you be more clear here?
cpp/src/community/k_truss_impl.cuh
Outdated
| std::nullopt, | ||
| std::nullopt, | ||
| cugraph::graph_properties_t{true, graph_view.is_multigraph()}, | ||
| true); | ||
| false); // FIXME: Renumbering should not be hardcoded. |
There was a problem hiding this comment.
I wasn't sure if renumbering should be hardcoded but I removed the FIXME.
… into branch-24.06_stable-edge_triangle-count
… into branch-24.06_stable-edge_triangle-count
…6_optimize-k-truss
… into branch-24.06_stable-edge_triangle-count
| std::move(std::get<0>(vertex_pair_buffer)), | ||
| std::move(std::get<1>(vertex_pair_buffer)), | ||
| std::nullopt, | ||
| // FIXME: Update 'shuffle_int_...' to support int32_t and int64_t values |
There was a problem hiding this comment.
Can you clarify this FIXME? You can use int32_t and int64_t values as you are currently doing this.
You are calling a function that was designed to shuffle vertex pairs along with edge weight, edge id and edge type around the GPU cluster. That's not what you have, you have vertex pairs and an increase count. If this function didn't exist, since you are in a .cu file you could replace this logic with something akin to this:
If your concern is that we should have a more general purpose function for shuffling vertex pairs and arbitrary attributes then be sure to specify what you want.
There was a problem hiding this comment.
You can use int32_t and int64_t values as you are currently doing this.
Right. This is currently how we proceed but based on Seunghwa feedbacks on decompress_edgelist, we should have something separate from edge_id to handle this in case edge_id and count don't hold the same type. I thought the same holds for the shuffling function.
There was a problem hiding this comment.
I guess I'm asking for a more descriptive FIXME. As worded it is not clear to me what you are asking to be changed at some point in the future.
There was a problem hiding this comment.
Right. I updated the FIXME and it will be available in the next commit
| mg_graph_to_sg_graph( | ||
| raft::handle_t const& handle, | ||
| cugraph::graph_view_t<vertex_t, edge_t, store_transposed, true> const& graph_view, | ||
| std::optional<cugraph::edge_property_view_t<edge_t, weight_t const*>> edge_weight_view, | ||
| std::optional<cugraph::edge_property_view_t<edge_t, edge_t const*>> edge_id_view, |
There was a problem hiding this comment.
Should we also add edge_type here, since we're modifying all of the functions? We do currently support edge weight, edge id and edge type in most of our functions.
… into branch-24.06_stable-edge_triangle-count
…6_stable-edge_triangle-count
…6_stable-edge_triangle-count
…6_stable-edge_triangle-count
…6_stable-edge_triangle-count
…gle-count' into branch-24.06_stable-edge_triangle-count
|
/merge |
This PR
closes #4370
closes #4371