Skip to content
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.

Sign up for GitHub

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

Fix bug in how is_symmetric is set when transposing storage #2898

Merged
merged 3 commits into from
Nov 9, 2022
+31 −6
Merged

Fix bug in how is_symmetric is set when transposing storage #2898

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
32ddd76
fix setting of graph properties during transpose_storage function
ChuckHastings Nov 9, 2022
e5389df
fix clang-format issues
ChuckHastings Nov 9, 2022
8f7d29f
noticed I left in some print statements, also same bug exists in tran…
ChuckHastings Nov 9, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 0 additions & 2 deletions cpp/src/c_api/core_number.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,8 +67,6 @@ struct core_number_functor : public cugraph::c_api::abstract_functor {
if (error_code_ != CUGRAPH_SUCCESS)
;
}
// FIXME: Transpose_storage may have a bug, since if store_transposed is True it can reverse
// the bool value of is_symmetric
auto graph =
reinterpret_cast<cugraph::graph_t<vertex_t, edge_t, weight_t, false, multi_gpu>*>(
graph_->graph_);
Expand Down
17 changes: 15 additions & 2 deletions cpp/src/structure/transpose_graph_storage_impl.cuh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,10 @@ transpose_graph_storage_impl(
}

auto is_multigraph = graph.is_multigraph();
auto is_symmetric = graph.is_symmetric();

// FIXME: if is_symmetric is true we can do this more efficiently,
// since the graph contents should be exactly the same

auto [edgelist_srcs, edgelist_dsts, edgelist_weights] =
decompress_to_edgelist(handle,
Expand Down Expand Up @@ -89,7 +93,7 @@ transpose_graph_storage_impl(
std::move(edgelist_dsts),
std::move(edgelist_weights),
std::nullopt,
graph_properties_t{is_multigraph, false},
graph_properties_t{is_symmetric, is_multigraph},
true);

return std::make_tuple(std::move(storage_transposed_graph), std::move(new_renumber_map));
Expand Down Expand Up @@ -123,6 +127,10 @@ transpose_graph_storage_impl(
auto number_of_vertices = graph.number_of_vertices();
auto is_multigraph = graph.is_multigraph();
bool renumber = renumber_map.has_value();
auto is_symmetric = graph.is_symmetric();

// FIXME: if is_symmetric is true we can do this more efficiently,
// since the graph contents should be exactly the same
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We may handle this inside the C++ function. We can check the flag and if symmetric, it might be possible to just move all the graph contents.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And why aren't you directly calling transpose_graph_storage? (https://github.com/rapidsai/cugraph/blob/branch-22.12/cpp/src/structure/transpose_graph_storage_impl.cuh#L168)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm confused by these comments. This is the C++ function.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops :-( Sorry... I thought this is the C API function...


auto [edgelist_srcs, edgelist_dsts, edgelist_weights] =
decompress_to_edgelist(handle,
Expand Down Expand Up @@ -150,9 +158,14 @@ transpose_graph_storage_impl(
std::move(edgelist_dsts),
std::move(edgelist_weights),
std::nullopt,
graph_properties_t{is_multigraph, false},
graph_properties_t{is_symmetric, is_multigraph},
renumber);

std::cout << "should have created graph with is_symmetric = " << (is_symmetric ? "TRUE" : "FALSE")
<< std::endl;
std::cout << " store_transposed_graph is_symmetric = "
<< (storage_transposed_graph.is_symmetric() ? "TRUE" : "FALSE") << std::endl;

return std::make_tuple(std::move(storage_transposed_graph), std::move(new_renumber_map));
}

Expand Down
19 changes: 19 additions & 0 deletions cpp/tests/c_api/weakly_connected_components_test.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -117,11 +117,30 @@ int test_weakly_connected_components()
return generic_wcc_test(h_src, h_dst, h_wgt, h_result, num_vertices, num_edges, FALSE);
}

int test_weakly_connected_components_transpose()
{
size_t num_edges = 32;
size_t num_vertices = 12;

vertex_t h_src[] = {0, 1, 1, 2, 2, 2, 3, 4, 6, 7, 7, 8, 8, 8, 9, 10,
1, 3, 4, 0, 1, 3, 5, 5, 7, 9, 10, 6, 7, 9, 11, 11};
vertex_t h_dst[] = {1, 3, 4, 0, 1, 3, 5, 5, 7, 9, 10, 6, 7, 9, 11, 11,
0, 1, 1, 2, 2, 2, 3, 4, 6, 7, 7, 8, 8, 8, 9, 10};
weight_t h_wgt[] = {1.0, 1.0, 1.0, 1.0, 1.0, 1.0, 1.0, 1.0, 1.0, 1.0, 1.0,
1.0, 1.0, 1.0, 1.0, 1.0, 1.0, 1.0, 1.0, 1.0, 1.0, 1.0,
1.0, 1.0, 1.0, 1.0, 1.0, 1.0, 1.0, 1.0, 1.0, 1.0};
vertex_t h_result[] = {0, 0, 0, 0, 0, 0, 1, 1, 1, 1, 1, 1};

// WCC wants store_transposed = FALSE
return generic_wcc_test(h_src, h_dst, h_wgt, h_result, num_vertices, num_edges, TRUE);
}

/******************************************************************************/

int main(int argc, char** argv)
{
int result = 0;
result |= RUN_TEST(test_weakly_connected_components);
result |= RUN_TEST(test_weakly_connected_components_transpose);
return result;
}