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.

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

Replace graph_view.hpp::number_of_edges with compute_number_of_edges #4026

Merged

Conversation

seunghwak
Copy link
Contributor

@seunghwak seunghwak commented Nov 29, 2023

Replace graph_view.hpp::number_of_edges (deprecated, throws an exception if an edge mask is attached to the graph view object) with compute_number_of_edges (this function works with or without edge mask)

seunghwak and others added 20 commits November 14, 2023 17:32
@seunghwak seunghwak requested a review from a team as a code owner November 29, 2023 18:15
@seunghwak seunghwak self-assigned this Nov 29, 2023
@seunghwak seunghwak added DO NOT MERGE Hold off on merging; see PR for details feature request New feature or request non-breaking Non-breaking change labels Nov 29, 2023
@seunghwak seunghwak added this to the 24.02 milestone Nov 29, 2023
@seunghwak seunghwak removed the DO NOT MERGE Hold off on merging; see PR for details label Nov 30, 2023
Copy link
Contributor

@jnke2016 jnke2016 left a comment

Choose a reason for hiding this comment

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

Looks good to me

Comment on lines +509 to +523
template <typename vertex_t, typename edge_t, bool store_transposed, bool multi_gpu>
edge_t graph_view_t<vertex_t, edge_t, store_transposed, multi_gpu, std::enable_if_t<!multi_gpu>>::
compute_number_of_edges(raft::handle_t const& handle) const
{
if (this->has_edge_mask()) {
auto value_firsts = (*(this->edge_mask_view())).value_firsts();
auto edge_counts = (*(this->edge_mask_view())).edge_counts();
assert(value_firsts.size() == 0);
assert(edge_counts.size() == 0);
return static_cast<edge_t>(detail::count_set_bits(handle, value_firsts[0], edge_counts[0]));
} else {
return this->number_of_edges_;
}
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need two specialization based on multi_gpu?
value_firsts.size() == 1 and isn't first one with for loop is good enough?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am not sure, let me experiment.

We have two specializations of graph_view_t based on multi_gpu. I am not sure we have two specializations of graph_view_t but have just one common member function definition.

This function no longer can't be in graph_base_t which is a parent class of graph_t & graph_view_t as masking is applied to graph_view_t but not on graph_t.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, it does not compile. graph_view_t<..., multi_gpu=true> & graph_view_t<..., multi_gpu=false> have different sets of member variables, and they are two different classes. We cannot merge two member functions of two different classes (even though the member function has the same name & code).

But thanks to your comment, I just realized that why I initially placed edge_mask_view_ in graph_view_t instead of graph_base_t. graph_base_t is for both graph_t & graph_view_t... and as graph_t cannot have a mask, I'd better move the edge_mask_view_ back to graph_view_t.

Let me make this change.

Copy link
Contributor

Choose a reason for hiding this comment

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

That makes a lot of sense.

Copy link
Contributor

@naimnv naimnv left a comment

Choose a reason for hiding this comment

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

LGTM

…r both graph_t & graph_view_t, graph_t can't have a mask)
@ChuckHastings
Copy link
Collaborator

/merge

@rapids-bot rapids-bot bot merged commit 7fe7bea into rapidsai:branch-24.02 Dec 12, 2023
74 checks passed
@seunghwak seunghwak deleted the fea_number_of_edges_with_mask branch May 22, 2024 17:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cuGraph feature request New feature or request non-breaking Non-breaking change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants