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

Refactor Python Neighborhood Sample #4988

Open
wants to merge 7 commits into
base: branch-25.04
Choose a base branch
from

Conversation

jnke2016
Copy link
Contributor

@jnke2016 jnke2016 commented Mar 19, 2025

closes #3855

@jnke2016 jnke2016 requested a review from a team as a code owner March 19, 2025 19:14
@jnke2016 jnke2016 self-assigned this Mar 19, 2025
@jnke2016 jnke2016 added this to the 25.04 milestone Mar 19, 2025
@jnke2016 jnke2016 marked this pull request as draft March 19, 2025 19:16
Copy link

copy-pr-bot bot commented Mar 19, 2025

Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually.

Contributors can view more details about this message here.

@rlratzel rlratzel added improvement Improvement / enhancement to an existing function non-breaking Non-breaking change labels Mar 20, 2025
@jnke2016 jnke2016 marked this pull request as ready for review March 20, 2025 12:03
@jnke2016 jnke2016 requested a review from a team as a code owner March 25, 2025 23:11
return start_list


def heterogeneous_uniform_neighbor_sample(
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this still have _uniform in the function name?

h_fan_out=fanout_vals,
num_edge_types=num_edge_types,
with_replacement=with_replacement,
do_expensive_check=True,
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be a parameter or at least hardcoded to False?

Comment on lines +292 to +293
if with_biases:
sampling_result_array_dict = pylibcugraph_heterogeneous_biased_neighbor_sample(
Copy link
Contributor

Choose a reason for hiding this comment

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

Are the calls exactly the same except for the PLC function itself? If so, you could trim this down quite a bit like this:

Suggested change
if with_biases:
sampling_result_array_dict = pylibcugraph_heterogeneous_biased_neighbor_sample(
if with_biases:
sampling_function = pylibcugraph_heterogeneous_biased_neighbor_sample
else:
sampling_function = pylibcugraph_heterogeneous_uniform_neighbor_sample

Then just call it once as

    sampling_result_array_dict = sampling_function( ... )

Copy link
Contributor

Choose a reason for hiding this comment

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

Was this committed by mistake?

Copy link
Contributor

Choose a reason for hiding this comment

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

Was this committed by mistake?

Copy link
Contributor

Choose a reason for hiding this comment

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

Was this committed by mistake?

Copy link
Contributor

Choose a reason for hiding this comment

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

Was this committed by mistake?

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 python
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove Old Sampling C API and PLC Functions
2 participants