Skip to content

[REVIEW]Fix Fanout -1#2358

Merged
rapids-bot[bot] merged 6 commits intorapidsai:branch-22.08from
VibhuJawa:fix_fanout_-1
Jun 16, 2022
Merged

[REVIEW]Fix Fanout -1#2358
rapids-bot[bot] merged 6 commits intorapidsai:branch-22.08from
VibhuJawa:fix_fanout_-1

Conversation

@VibhuJawa
Copy link
Member

@VibhuJawa VibhuJawa commented Jun 15, 2022

This PR fixes sampling when for the default value of -1 which is called below:

filtered_list = sample_groups(filtered_list,
by="dst",
n_samples=fanout)
return filtered_list['dst'].values, filtered_list['src'].values

In workflows this is called during inference so we need this to work for inference to work .with CuGraphStorage .

@VibhuJawa VibhuJawa added the bug Something isn't working label Jun 15, 2022
@VibhuJawa VibhuJawa added the non-breaking Non-breaking change label Jun 15, 2022
@VibhuJawa VibhuJawa changed the title [WIP]Fix Fanout -1 [REVIEW]Fix Fanout -1 Jun 16, 2022
@VibhuJawa VibhuJawa marked this pull request as ready for review June 16, 2022 00:03
@VibhuJawa VibhuJawa requested a review from a team as a code owner June 16, 2022 00:03
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.

The code change and test LGTM and I'll just approve now, but I have one small request: could you also update the docstring here to indicate that -1 now returns all samples?

@codecov-commenter
Copy link

codecov-commenter commented Jun 16, 2022

Codecov Report

❗ No coverage uploaded for pull request base (branch-22.08@c837aaa). Click here to learn what that means.
The diff coverage is n/a.

@@               Coverage Diff               @@
##             branch-22.08    #2358   +/-   ##
===============================================
  Coverage                ?   60.76%           
===============================================
  Files                   ?      105           
  Lines                   ?     5077           
  Branches                ?        0           
===============================================
  Hits                    ?     3085           
  Misses                  ?     1992           
  Partials                ?        0           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c837aaa...7fda2b2. Read the comment docs.

@VibhuJawa
Copy link
Member Author

VibhuJawa commented Jun 16, 2022

The code change and test LGTM and I'll just approve now, but I have one small request: could you also update the docstring here to indicate that -1 now returns all samples?

Added that. Also for full visibility this makes it align with the DGL behavior mentioned here.

https://github.com/dmlc/dgl/blob/b258729b3f0613ef685d40b1ad51a84f455745c5/python/dgl/sampling/neighbor.py#L182-L183

@rlratzel
Copy link
Contributor

@gpucibot merge

@rapids-bot rapids-bot bot merged commit df82b74 into rapidsai:branch-22.08 Jun 16, 2022
nv-rliu pushed a commit to nv-rliu/cugraph-gnn that referenced this pull request Jul 15, 2025
This PR fixes sampling  when for the default value of -1 which is called below: 

https://github.com/rapidsai/cugraph/blob/92f6ba451b2d9e6c3f60dbccfa05bbf3c480e43a/python/cugraph/cugraph/gnn/graph_store.py#L129-L133

In workflows this is called during inference so we need this to work for inference to work .with CuGraphStorage .

Authors:
  - Vibhu Jawa (https://github.com/VibhuJawa)

Approvers:
  - Rick Ratzel (https://github.com/rlratzel)
  - Alex Barghi (https://github.com/alexbarghi-nv)

URL: rapidsai/cugraph#2358
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working non-breaking Non-breaking change

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants