Skip to content

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
@mszarma mszarma requested a review from rusty1s February 20, 2023 08:14
@mszarma mszarma marked this pull request as ready for review February 20, 2023 08:17
@mszarma mszarma force-pushed the trim_to_layer_pr_draft branch from 6d3486e to 5209199 Compare February 20, 2023 08:35
@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.

@mszarma mszarma force-pushed the trim_to_layer_pr_draft branch from 5d34fe4 to 7f56bcc Compare February 27, 2023 09:00
@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