Skip to content

[IMP] Match Default PyG Hop ID Behavior in cuGraph-PyG#3565

Merged
rapids-bot[bot] merged 22 commits intorapidsai:branch-23.06from
alexbarghi-nv:cugraph-pyg-hops
May 22, 2023
Merged

[IMP] Match Default PyG Hop ID Behavior in cuGraph-PyG#3565
rapids-bot[bot] merged 22 commits intorapidsai:branch-23.06from
alexbarghi-nv:cugraph-pyg-hops

Conversation

@alexbarghi-nv
Copy link
Member

@alexbarghi-nv alexbarghi-nv commented May 16, 2023

Closes #3563
Closes #3564

Merge after #3567

@alexbarghi-nv alexbarghi-nv self-assigned this May 16, 2023
@alexbarghi-nv alexbarghi-nv added improvement Improvement / enhancement to an existing function breaking Breaking change labels May 16, 2023
@alexbarghi-nv alexbarghi-nv added this to the 23.06 milestone May 16, 2023
@alexbarghi-nv alexbarghi-nv added the Blocked Cannot progress due to external reasons label May 17, 2023
@alexbarghi-nv alexbarghi-nv marked this pull request as ready for review May 17, 2023 18:01
@alexbarghi-nv alexbarghi-nv requested review from a team as code owners May 17, 2023 18:01
@alexbarghi-nv alexbarghi-nv removed the Blocked Cannot progress due to external reasons label May 18, 2023
@alexbarghi-nv alexbarghi-nv removed the request for review from a team May 18, 2023 04:47
Copy link
Contributor

@rlratzel rlratzel left a comment

Choose a reason for hiding this comment

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

LGTM


index = cugraph_store._get_vertex_groups_from_sample(nodes_of_interest)
assert index["vt1"].tolist() == sorted(nodes_of_interest.tolist())
assert index["vt1"].tolist() == nodes_of_interest.tolist()
Copy link
Contributor

Choose a reason for hiding this comment

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

No suggestion here, but I'm curious why sorted() was removed (or why it wasn't added to both lists being compared), since wouldn't that be a more reliable compare? Or, does order matter?

Copy link
Member Author

Choose a reason for hiding this comment

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

Order does matter now, that is one of the changes in this PR. Before, the nodes of interest were sorted, but that is not correct - they should be returned in order of appearance.

@alexbarghi-nv
Copy link
Member Author

/merge

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

breaking Breaking change improvement Improvement / enhancement to an existing function

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Update cuGraph-PyG to Renumber Edge Index using Hash Table Update cuGraph-PyG to Label Nodes by Hop ID and Return Sample Sizes

3 participants