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

Jump to bottom

Biased RW implementation and test #4499

Closed
wants to merge 31 commits into from
Closed

Biased RW implementation and test #4499

wants to merge 31 commits into from

Conversation

G-Cornett
Copy link

@G-Cornett G-Cornett commented Jun 24, 2024 •
edited by ChuckHastings
Loading

Biased random walk selector implemented. Tests changed to accurately reflect expected behavior.

closes #2555

@G-Cornett G-Cornett requested a review from a team as a code owner June 24, 2024 21:30
@copy-pr-bot copy-pr-bot
Copy link

copy-pr-bot bot commented Jun 24, 2024

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

@ChuckHastings ChuckHastings added improvement Improvement / enhancement to an existing function non-breaking Non-breaking change labels Jun 24, 2024
@ChuckHastings
Copy link
Collaborator

/ok to test

seunghwak
seunghwak reviewed Jun 25, 2024
View reviewed changes
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.

Congratulations on your first PR!!!

cpp/src/sampling/random_walks_impl.cuh Outdated Show resolved Hide resolved
cpp/src/sampling/random_walks_impl.cuh Show resolved Hide resolved
cpp/src/sampling/random_walks_impl.cuh Outdated Show resolved Hide resolved
cpp/src/sampling/random_walks_impl.cuh Show resolved Hide resolved
seunghwak
seunghwak reviewed Jul 10, 2024
View reviewed changes
cpp/include/cugraph/algorithms.hpp Outdated Show resolved Hide resolved
cpp/src/sampling/random_walks_impl.cuh Outdated Show resolved Hide resolved
cpp/src/sampling/random_walks_impl.cuh Show resolved Hide resolved
cpp/src/sampling/random_walks_impl.cuh Outdated Show resolved Hide resolved
@G-Cornett
Copy link
Author

G-Cornett commented Jul 23, 2024 •
edited
Loading

@seunghwak @ChuckHastings
As requested, here is the sanitized node2vec implementation and generic refactoring. Node2Vec, as currently written, passes the karate dataset test but fails larger tests when in a multi-GPU environment

@BradReesWork BradReesWork changed the base branch from branch-24.08 to branch-24.10 August 5, 2024 19:23
@BradReesWork BradReesWork added this to the 24.10 milestone Aug 5, 2024
@ChuckHastings
Copy link
Collaborator

/okay to test

ChuckHastings
ChuckHastings reviewed Aug 8, 2024
View reviewed changes
Copy link
Collaborator

@ChuckHastings ChuckHastings left a comment

Choose a reason for hiding this comment

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

Great job!

A number of things to improve the quality. A question for @seunghwak to perhaps improve code reuse.

cpp/include/cugraph/algorithms.hpp Outdated Show resolved Hide resolved
cpp/include/cugraph/algorithms.hpp Outdated Show resolved Hide resolved
cpp/include/cugraph/algorithms.hpp Outdated Show resolved Hide resolved
cpp/src/c_api/random_walks.cpp Outdated Show resolved Hide resolved
cpp/src/c_api/random_walks.cpp Outdated Show resolved Hide resolved
cpp/src/c_api/random_walks.cpp Outdated Show resolved Hide resolved
cpp/src/c_api/random_walks.cpp Outdated Show resolved Hide resolved
cpp/src/sampling/random_walks_impl.cuh Outdated Show resolved Hide resolved
cpp/src/sampling/random_walks_impl.cuh Outdated Show resolved Hide resolved
cpp/src/sampling/random_walks_impl.cuh Show resolved Hide resolved
seunghwak
seunghwak reviewed Aug 9, 2024
View reviewed changes
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.

Review Part 1

cpp/src/sampling/random_walks_impl.cuh Outdated Show resolved Hide resolved
cpp/src/sampling/random_walks_impl.cuh Show resolved Hide resolved
cpp/src/sampling/random_walks_impl.cuh Show resolved Hide resolved
cpp/src/sampling/random_walks_impl.cuh Outdated Show resolved Hide resolved
cpp/src/sampling/random_walks_impl.cuh Outdated Show resolved Hide resolved
cpp/src/sampling/random_walks_impl.cuh Outdated Show resolved Hide resolved
cpp/src/sampling/random_walks_impl.cuh Outdated Show resolved Hide resolved
cpp/src/sampling/random_walks_impl.cuh Outdated Show resolved Hide resolved
cpp/src/sampling/random_walks_impl.cuh Outdated Show resolved Hide resolved
cpp/src/sampling/random_walks_impl.cuh Outdated Show resolved Hide resolved
seunghwak
seunghwak reviewed Aug 9, 2024
View reviewed changes
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.

Review Part 2.

cpp/src/sampling/random_walks_impl.cuh Outdated Show resolved Hide resolved
cpp/src/sampling/random_walks_impl.cuh Outdated Show resolved Hide resolved
cpp/src/sampling/random_walks_impl.cuh Outdated Show resolved Hide resolved
cpp/src/sampling/random_walks_impl.cuh Outdated Show resolved Hide resolved
seunghwak
seunghwak approved these changes Aug 27, 2024
View reviewed changes
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.

I have few suggestions for minor fixes. Otherwise, looks good to me and thank you for your contribution!!!

cpp/src/sampling/random_walks_impl.cuh Outdated Show resolved Hide resolved
cpp/src/sampling/random_walks_impl.cuh Outdated Show resolved Hide resolved
cpp/src/sampling/random_walks_impl.cuh Outdated Show resolved Hide resolved
cpp/src/sampling/random_walks_impl.cuh Outdated Show resolved Hide resolved
cpp/src/sampling/random_walks_impl.cuh Outdated Show resolved Hide resolved
@ChuckHastings
Copy link
Collaborator

/okay to test

ChuckHastings
ChuckHastings approved these changes Aug 30, 2024
View reviewed changes
Copy link
Collaborator

@ChuckHastings ChuckHastings left a comment

Choose a reason for hiding this comment

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

Great job @G-Cornett. Thanks for your work this summer!

@ChuckHastings
Copy link
Collaborator

/ok to test

@ChuckHastings ChuckHastings mentioned this pull request Sep 4, 2024
Merged
@rapids-bot rapids-bot bot closed this in #4645 Sep 16, 2024
@rapids-bot rapids-bot bot closed this in 4d2dd27 Sep 16, 2024
@ChuckHastings ChuckHastings mentioned this pull request Sep 16, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@seunghwak seunghwak seunghwak approved these changes

@ChuckHastings ChuckHastings ChuckHastings approved these changes

Assignees
No one assigned
Labels
cuGraph improvement Improvement / enhancement to an existing function non-breaking Non-breaking change
Projects
None yet
Milestone
24.10
Development

Successfully merging this pull request may close these issues.

MNMG Random Walks - Implementation
4 participants
@G-Cornett @ChuckHastings @seunghwak @BradReesWork