Bi-directional Sampling for NeighborSampler #10126
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
zaristei
force-pushed
the
zaristei/bidirectional-sampler
branch
5 times, most recently
from
37688a8 to
7ba87d5
Compare
This was referenced Mar 25, 2025
Contributor
|
looks good so far |
puririshi98
added a commit
that referenced
this pull request
Apr 2, 2025
While working on [this PR](#10126), I noticed that there are no sanity tests for the default NeighborSampler. Although NeigborLoader effectively tests a lot of this functionality, having lower level tests helps with ensuring consistency for each `sample` call of the sampler, without the batching logic of `NeighborLoader` getting involved. --------- Co-authored-by: Zack Aristei <[email protected]> Co-authored-by: Rishi Puri <[email protected]>
zaristei
force-pushed
the
zaristei/bidirectional-sampler
branch
2 times, most recently
from
fdf6450 to
c9eab32
Compare
zaristei
force-pushed
the
zaristei/bidirectional-sampler
branch
2 times, most recently
from
d2506c8 to
9adc7b9
Compare
Contributor
|
closing since matthias said theres an existing simple way to do this. please jsut put the fix into my branch: |
puririshi98
requested changes
Apr 15, 2025
Contributor
puririshi98
left a comment
puririshi98
left a comment
There was a problem hiding this comment.
LGTM but i will request that @akihironitta or @rusty1s review this
zaristei
force-pushed
the
zaristei/bidirectional-sampler
branch
2 times, most recently
from
e288d28 to
d99fb71
Compare
Contributor
Author
|
#10200 should be merged first |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #10126 +/- ##
==========================================
- Coverage 86.11% 85.90% -0.21%
==========================================
Files 496 501 +5
Lines 33655 34811 +1156
==========================================
+ Hits 28981 29906 +925
- Misses 4674 4905 +231 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
chrisn-pik
pushed a commit
to chrisn-pik/pytorch_geometric
that referenced
this pull request
Jun 30, 2025
…am#10139) While working on [this PR](pyg-team#10126), I noticed that there are no sanity tests for the default NeighborSampler. Although NeigborLoader effectively tests a lot of this functionality, having lower level tests helps with ensuring consistency for each `sample` call of the sampler, without the batching logic of `NeighborLoader` getting involved. --------- Co-authored-by: Zack Aristei <[email protected]> Co-authored-by: Rishi Puri <[email protected]>
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
puririshi98
approved these changes
Aug 22, 2025
Contributor
puririshi98
left a comment
puririshi98
left a comment
There was a problem hiding this comment.
merging as per discussions with matthias, these are heavily tested through NVIDIA already
Contributor
|
turns out this PR already included fixes from the other PR and so when they merged it duplicated them. reverting the merge just for that file seemed to fix things since the changes from his older pr still exist in main, should be good to merge once the unrelated onnx issue is fixed. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR makes the following changes:
srctodstnode, and not just fromdsttosrcnode, which is the current default behavior.BidirectionalNeighborSampler, which samples nodes/edges in both upstream and downstream directions for a directed graph, as if it were an undirected graph.merge_withandcollatemethods to the SamplerOutput dataclass, allowing for different SamplerOutputs to be merged together. This also becomes useful in scenarios where the user may want to sample using multiple different unique sampling methods.This PR will remain in draft until
methods are also implemented on the corresponding Hetero graph classes, andunittests have been written to cover new behaviors.UPDATE: Hetero Bidirectional Sampler and SamplerOutput merge will be a separate PR.