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

Fix bug in MG Neighborhood sampling #4827

Conversation

jnke2016
Copy link
Contributor

This PR fixes a bug when computing the global start vertex labels

@jnke2016 jnke2016 requested a review from a team as a code owner December 10, 2024 22:29
@@ -948,11 +948,13 @@ struct neighbor_sampling_functor : public cugraph::c_api::abstract_functor {
std::exclusive_scan(
recvcounts.begin(), recvcounts.end(), displacements.begin(), size_t{0});

(*label_to_comm_rank)
.resize(displacements.back() + recvcounts.back(), handle_.get_stream());
rmm::device_uvector<label_t> tmp_label_to_comm_rank(
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why not just use label_to_comm_rank.resize(...) here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is an optional which is initialized to std::nullopt

Copy link
Contributor

Choose a reason for hiding this comment

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

label_to_comm_rank =
  rmm::device_uvector<label_t>(displacements.back() + recvcounts.back(), handle_.get_stream());

is sufficient (no need to create a temporary and move).

Copy link
Member

@alexbarghi-nv alexbarghi-nv left a comment

Choose a reason for hiding this comment

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

👍

@alexbarghi-nv alexbarghi-nv added bug Something isn't working non-breaking Non-breaking change labels Dec 17, 2024
@alexbarghi-nv alexbarghi-nv added this to the 25.02 milestone Dec 17, 2024
@alexbarghi-nv alexbarghi-nv requested a review from a team December 17, 2024 18:23
Copy link
Contributor

@seunghwak seunghwak left a comment

Choose a reason for hiding this comment

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

Besides the above comment, looks good to me.

@jnke2016 jnke2016 force-pushed the branch-25.02_fix-bug-mg-nbr-sampling branch from 5a1679b to 1ca214b Compare January 6, 2025 17:59
@ChuckHastings
Copy link
Collaborator

/merge

@rapids-bot rapids-bot bot merged commit c522baf into rapidsai:branch-25.02 Jan 7, 2025
73 checks passed
rapids-bot bot pushed a commit that referenced this pull request Jan 13, 2025
Adds support for heterogeneous distributed sampling to the cuGraph distributed sampler.  Prerequisite for exposing this functionality to cuGraph-PyG.  Has been initially tested with cuGraph-PyG.

Updates the distributed sampler to use the new sampling API.

Merge after #4775, #4827, #4820

Closes #4773 
Closes #4401

Authors:
  - Alex Barghi (https://github.com/alexbarghi-nv)
  - Joseph Nke (https://github.com/jnke2016)
  - Ralph Liu (https://github.com/nv-rliu)

Approvers:
  - Rick Ratzel (https://github.com/rlratzel)

URL: #4795
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working cuGraph non-breaking Non-breaking change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants