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

add API for the new uniform neighborhood sampling #2236

Merged
merged 1 commit into from
Apr 27, 2022

Conversation

ChuckHastings
Copy link
Collaborator

@ChuckHastings ChuckHastings commented Apr 21, 2022

Added an API for the new uniform neighborhood sampling implementation.

Added the API with an experimental tag in its name so it won't break the current python code. Plan is to have the new API replace the existing API during this release.

Plan for this release will be to have the edge weight be an edge index (passed by the calling python code), so the return "index" value will be the edge weight. Down the road we will release this restriction and allow real edge weights.

Addresses part 1 of #2226

@ChuckHastings ChuckHastings requested a review from a team as a code owner April 21, 2022 02:16
@ChuckHastings ChuckHastings self-assigned this Apr 21, 2022
@ChuckHastings ChuckHastings added 3 - Ready for Review improvement Improvement / enhancement to an existing function non-breaking Non-breaking change labels Apr 21, 2022
@ChuckHastings ChuckHastings added this to the 22.06 milestone Apr 21, 2022
@seunghwak
Copy link
Contributor

seunghwak commented Apr 21, 2022

@ChuckHastings So, in using the wedge weight as the edge index, are you planning to set weight_t to integer type? Or you plan to store integer in floating point format? I guess the latter will be less invasive (assuming there is no resolution issue for a very large number of edges).

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.

LGTM.

@ChuckHastings
Copy link
Collaborator Author

@ChuckHastings So, in using the wedge weight as the edge index, are you planning to set weight_t to integer type? Or you plan to store integer in floating point format? I guess the latter will be less invasive (assuming there is no resolution issue for a very large number of edges).

I was imagining I would instantiate graph_t<int32_t, int32_t, int32_t>, graph_t<int32_t, int64_t, int64_t> and graph_t<int64_t, int64_t, int64_t and use them for use in only this algorithm. But if the cost is too great in the implementation - since we do no computation using the weight values for this algorithm - we could use the float for int32 and the double for int64 and everything should work fine. I'll work that out during implementation.

@ChuckHastings
Copy link
Collaborator Author

rerun tests

1 similar comment
@ChuckHastings
Copy link
Collaborator Author

rerun tests

@ChuckHastings
Copy link
Collaborator Author

@gpucibot merge

@rapids-bot rapids-bot bot merged commit 2956d56 into rapidsai:branch-22.06 Apr 27, 2022
@ChuckHastings ChuckHastings deleted the fea_sg_nbr_sampling branch August 4, 2022 18:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
improvement Improvement / enhancement to an existing function non-breaking Non-breaking change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants