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

Enforce matching type #4161

Merged

Conversation

jnke2016
Copy link
Contributor

@jnke2016 jnke2016 commented Feb 9, 2024

This PR enforces that the vertex and the start_vertices are of the same type and throws a warning in the python API and an exception in the CAPI when there is a mismatch.

closes #4094

@jnke2016 jnke2016 requested a review from a team as a code owner February 9, 2024 18:31
@github-actions github-actions bot added the python label Feb 9, 2024
@jnke2016 jnke2016 requested a review from a team as a code owner February 10, 2024 08:51
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 otherwise

@naimnv naimnv requested review from benfred, rlratzel and ChuckHastings and removed request for benfred February 13, 2024 19:25
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.

Approving the C/C++ part

@jnke2016 jnke2016 requested a review from naimnv March 10, 2024 17:55
python/cugraph/cugraph/sampling/node2vec.py Outdated Show resolved Hide resolved
python/cugraph/cugraph/sampling/node2vec.py Outdated Show resolved Hide resolved
@@ -165,8 +163,22 @@ def test_node2vec(
)
num_verts = G.number_of_vertices()
k = random.randint(6, 12)
start_vertices = cudf.Series(random.sample(range(num_verts), k), dtype="int32")
start_vertices = cudf.Series(
random.sample(range(num_verts), k), dtype=start_vertices_type
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe this is a FIXME, but if we're picking samples at random it could make it hard to repro a test failure. We should somehow make it so tests with specific samples can be re-run in the event that the test fails.

Copy link
Contributor Author

@jnke2016 jnke2016 Mar 13, 2024

Choose a reason for hiding this comment

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

Right. The samples are random so that the test doesn't cover a fixed set of start vertices but I do agree it makes it hard to debug. Perhaps we can add a print those start_vertices for debugging purposes? I added a FIXME regarding this.

@BradReesWork BradReesWork added improvement Improvement / enhancement to an existing function non-breaking Non-breaking change labels Mar 12, 2024
@BradReesWork BradReesWork added this to the 24.04 milestone Mar 12, 2024
@rlratzel
Copy link
Contributor

/merge

@rapids-bot rapids-bot bot merged commit 728ffd0 into rapidsai:branch-24.04 Mar 13, 2024
136 of 137 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cuGraph improvement Improvement / enhancement to an existing function non-breaking Non-breaking change python
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[FEA]: Implementing cuGraph.node2vec on kaggel or other datasets
6 participants