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

Adding option to return info about sampled graph #197

Merged
merged 12 commits into from
Feb 28, 2023

Conversation

mszarma
Copy link
Contributor

@mszarma mszarma commented Feb 10, 2023

It's enabling the hierarchical tensor usage
and significant performance improvement

PyG part: pyg-team/pytorch_geometric#6661

It's enabling the hierarchical tensor usage
and significant performance improvement
@codecov-commenter
Copy link

codecov-commenter commented Feb 20, 2023

Codecov Report

Merging #197 (1f0a3f0) into master (1760817) will increase coverage by 0.11%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #197      +/-   ##
==========================================
+ Coverage   83.37%   83.49%   +0.11%     
==========================================
  Files          26       26              
  Lines         842      848       +6     
==========================================
+ Hits          702      708       +6     
  Misses        140      140              
Impacted Files Coverage Δ
pyg_lib/csrc/sampler/neighbor.cpp 100.00% <ø> (ø)
pyg_lib/csrc/sampler/cpu/neighbor_kernel.cpp 88.72% <100.00%> (+0.53%) ⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Copy link
Member

@rusty1s rusty1s left a comment

Choose a reason for hiding this comment

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

I think code-wise this looks all good. Remaining questions would be

  • Can we clean-up dispatching logic? Do you have any solution for this?
  • Would prefer some renaming ala num_nodes_per_hop or similar
  • I'm not totally sure why we gate behind disjoint in the heterogeneous case but not in the homogeneous one.

pyg_lib/sampler/__init__.py Outdated Show resolved Hide resolved
pyg_lib/sampler/__init__.py Outdated Show resolved Hide resolved
pyg_lib/csrc/sampler/cpu/neighbor_kernel.cpp Outdated Show resolved Hide resolved
pyg_lib/csrc/sampler/cpu/neighbor_kernel.cpp Outdated Show resolved Hide resolved
pyg_lib/csrc/sampler/cpu/neighbor_kernel.cpp Outdated Show resolved Hide resolved
pyg_lib/csrc/sampler/cpu/neighbor_kernel.cpp Outdated Show resolved Hide resolved
pyg_lib/csrc/sampler/cpu/neighbor_kernel.cpp Outdated Show resolved Hide resolved
pyg_lib/csrc/sampler/cpu/neighbor_kernel.cpp Outdated Show resolved Hide resolved
@mszarma
Copy link
Contributor Author

mszarma commented Feb 27, 2023

I think code-wise this looks all good. Remaining questions would be

  • Can we clean-up dispatching logic? Do you have any solution for this?
  • Would prefer some renaming ala num_nodes_per_hop or similar
  • I'm not totally sure why we gate behind disjoint in the heterogeneous case but not in the homogeneous one.

Thanks you @rusty1s for the review.

  • Can we clean-up dispatching logic? Do you have any solution for this?
    I would recommend to put it to the PyG backlog and then think how to tackle that. Would prefer not to mix it. Move forward the feature part separately and then as separate task think how to refactor that dispatching logic. My gut feeling is that it may need a major refactor.
  • Would prefer some renaming ala num_nodes_per_hop or similar
    That's a good idea. Done.
  • I'm not totally sure why we gate behind disjoint in the heterogeneous case but not in the homogeneous one.
    The difference in hetero flow is that we are using the dict/Phmap. The issue is in usage of node_t: https://github.com/pyg-team/pyg-lib/blob/master/pyg_lib/csrc/sampler/cpu/neighbor_kernel.cpp#L220. In the disjoint hetero flow compiler point out that phmap cannot match/use the pair type. I didn't investigated usage of disjoint/temporal flow so my recommendation would be to enable feature firstly without disjoint support. WDYT @rusty1s ?

@rusty1s rusty1s enabled auto-merge (squash) February 28, 2023 09:25
@rusty1s rusty1s merged commit c04fb60 into pyg-team:master Feb 28, 2023
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.

3 participants