-
Notifications
You must be signed in to change notification settings - Fork 334
add C implementation of MST #5044
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
cpp/src/c_api/legacy_mst.cpp
Outdated
namespace c_api { | ||
|
||
struct cugraph_layout_result_t { | ||
cugraph_type_erased_device_array_t* vertices_{nullptr}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd suggest using cugraph_induced_subgraph_result_t
as the return type... as that has a COO of edges.
We should probably think of a better name, but perhaps a FIXME on that for now. I have a branch where I've been working on C API updates in graph creation that I think will create a better name for this structure.
|
||
const size_t num_edges = result_legacy_coo_graph->view().number_of_edges; | ||
|
||
rmm::device_uvector<vertex_t> result_src(num_edges, handle_.get_stream()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you add a FIXME here.
These copies could be unnecessary. src_indices
, dst_indices
and edge_data
within the result_legacy_coo_graph are already rmm::device_buffer
instances. If we made a new constructor for cugraph_type_erased_host_array_t
that took an rmm::device_buffer
we could skip these copies.
We can do that optimization later.
cpp/tests/c_api/legacy_mst_test.c
Outdated
@@ -0,0 +1,227 @@ | |||
/* | |||
* Copyright (c) 2023-2024, NVIDIA CORPORATION. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should just be 2025
cpp/tests/traversal/ms_bfs_test.cu
Outdated
d_distances.end(), | ||
static_cast<vertex_t>(0)); | ||
ASSERT_TRUE(ref_sum > 0); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this update in this PR? Seems out of place.
Is this just a matter of merging the MSBFS PR first?
|
||
def minimum_spanning_tree(ResourceHandle resource_handle, | ||
_GPUGraph graph, | ||
bool_t do_expensive_check): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can this have a default value of False?
[0 1 2 3 4 5 1 1 1 4] | ||
>>> destinations | ||
[1 0 1 1 1 4 2 3 4 5] | ||
>>> edge_weights | ||
[0.1 0.1 3.1 2.1 1.1 3.2 3.1 2.1 1.1 3.2] | ||
>>> subgraph_offsets | ||
[0 10] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this the actual output of running the example code? These don't look like device array reprs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
else: | ||
cupy_edge_weights = None | ||
|
||
# FIXME: Should we keep the offsets array or just drop it from the final |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we resolve this in this PR?
/merge |
This PR adds the C, PLC and python API for Minimum Spanning Tree
closes #4882