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

Update DistNeighborSampler for homo graphs [2/6] #8209

Merged
merged 13 commits into from
Nov 10, 2023
Merged

Conversation

kgajdamo
Copy link
Contributor

@kgajdamo kgajdamo commented Oct 17, 2023

Changes made:

  • added support for temporal sampling
  • use torch.Tensors instead of numpy arrays
  • move _sample_one_hop() from NeighborSampler to DistNeighborSampler
  • do not go with disjoint flow in _sample() function - this is not needed because batch is calculated after
  • added tests for node sampling and disjoint (works without DistNeighborLoader)
  • added tests for node temporal sampling (works without DistNeighborLoader)
  • some minor changes like changing variables names etc

This PR is based on the #8083, so both must be combined to pass the tests.

Other distributed PRs:
#8083
#8080
#8085

@codecov
Copy link

codecov bot commented Oct 17, 2023

Codecov Report

Merging #8209 (765821c) into master (9ea2233) will increase coverage by 0.91%.
The diff coverage is 72.28%.

❗ Current head 765821c differs from pull request most recent head 6e1bf95. Consider uploading reports for the commit 6e1bf95 to get more accurate results

@@            Coverage Diff             @@
##           master    #8209      +/-   ##
==========================================
+ Coverage   87.17%   88.08%   +0.91%     
==========================================
  Files         473      473              
  Lines       28757    28774      +17     
==========================================
+ Hits        25068    25345     +277     
+ Misses       3689     3429     -260     
Files Coverage Δ
torch_geometric/distributed/local_graph_store.py 96.93% <100.00%> (+1.06%) ⬆️
torch_geometric/sampler/neighbor_sampler.py 92.63% <87.50%> (+6.16%) ⬆️
...rch_geometric/distributed/dist_neighbor_sampler.py 58.00% <70.27%> (+58.00%) ⬆️

... and 6 files with indirect coverage changes

📣 Codecov offers a browser extension for seamless coverage viewing on GitHub. Try it in Chrome or Firefox today!

@JakubPietrakIntel JakubPietrakIntel force-pushed the intel/dist-sampler-4 branch 2 times, most recently from 5fe69fb to b7acc09 Compare October 30, 2023 12:21
@rusty1s rusty1s changed the title Update DistNeighborSampler for homo graphs Update DistNeighborSampler for homo graphs [2/6] Oct 30, 2023
@JakubPietrakIntel JakubPietrakIntel force-pushed the intel/dist-sampler-4 branch 5 times, most recently from 530b9c7 to d4b25a3 Compare November 3, 2023 17:24
@rusty1s rusty1s enabled auto-merge (squash) November 10, 2023 10:45
@rusty1s rusty1s merged commit f4ed34c into master Nov 10, 2023
14 checks passed
@rusty1s rusty1s deleted the intel/dist-sampler-4 branch November 10, 2023 10:49
@@ -119,7 +119,10 @@ def __init__(
feature_store, graph_store = data

# Obtain graph metadata:
node_attrs = feature_store.get_all_tensor_attrs()
node_attrs = [
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@rusty1s Thank you for the review :). One comment about these lines: they are common for both homo and hetero. In case of homo the attr.group_name will be None and therefore the condition isinstance(attr.group_name, NodeType) will never be met and node_attrs will be always empty. I will open a new PR with the previous version.

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

Successfully merging this pull request may close these issues.

2 participants