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

Add support for projecting features before aggregation in SAGEConv #4437

Merged
merged 3 commits into from
Apr 10, 2022

Conversation

wilcoln
Copy link
Contributor

@wilcoln wilcoln commented Apr 9, 2022

This pull request adds the implementation for Max and Mean Pool aggregations for SAGEConv which were implemented in the original study Inductive Representation Learning on Large Graphs, it's a sequel to PR #4379, which in turn is related to issue #1147.

The implementation is a direct application of the Eq 3 of the original paper (for both mean and max):
image

cc @hunarbatra

@codecov
Copy link

codecov bot commented Apr 9, 2022

Codecov Report

Merging #4437 (eca3e49) into master (3f0019f) will decrease coverage by 0.05%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #4437      +/-   ##
==========================================
- Coverage   82.69%   82.63%   -0.06%     
==========================================
  Files         313      313              
  Lines       16240    16259      +19     
==========================================
+ Hits        13429    13435       +6     
- Misses       2811     2824      +13     
Impacted Files Coverage Δ
torch_geometric/nn/conv/sage_conv.py 100.00% <100.00%> (ø)
torch_geometric/data/hetero_data.py 93.10% <0.00%> (-2.30%) ⬇️
torch_geometric/data/data.py 90.70% <0.00%> (-1.41%) ⬇️
torch_geometric/loader/link_neighbor_loader.py 89.52% <0.00%> (-0.80%) ⬇️

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 3f0019f...eca3e49. Read the comment docs.

@wilcoln wilcoln force-pushed the sageconv-pool branch 4 times, most recently from 9e4f67b to a804b4a Compare April 9, 2022 03:00
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.

Thank you for the PR. I am not sure I fully understand this. How does that differ from aggr=mean and aggr=max? IMO, these already implement the linked Equation (we just added LSTM support to make SAGEConv finally support all proposed aggregations).

@wilcoln
Copy link
Contributor Author

wilcoln commented Apr 9, 2022

Thanks for your comment @rusty1s .

As you know, the mean/max aggregator directly applies the mean/max reduction to all neighboring nodes.

While the mean/max pool aggregator first applies an additional linear layer (+ bias) followed by an activation function, and only then, performs the element-wise mean/max reduction.

@wilcoln wilcoln force-pushed the sageconv-pool branch 2 times, most recently from f4bc063 to 3b96491 Compare April 9, 2022 14:20
@wilcoln wilcoln requested a review from rusty1s April 9, 2022 16:59
@rusty1s
Copy link
Member

rusty1s commented Apr 9, 2022

Thanks for the clarification. How about we add an option to apply the transformation before aggregation rather than after it? This way, there would be no need to modify the underlying aggregation scheme.

@wilcoln
Copy link
Contributor Author

wilcoln commented Apr 9, 2022

That would be much cleaner indeed and would work with all existing aggregators, I will update my PR accordingly, by adding a pool boolean parameter to the class constructor, defaulting to False. How does that sound?

@rusty1s
Copy link
Member

rusty1s commented Apr 9, 2022

Sounds good to me, thank you very much! I am wondering if pool is the most descriptive name for this. How about we name it pre_transform or something similar?

@rusty1s rusty1s changed the title Add support for mean and max pool in SAGEConv Add support for projecting features before aggregation in SAGEConv Apr 10, 2022
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.

Thank you!

@rusty1s rusty1s merged commit ea0bff1 into pyg-team:master Apr 10, 2022
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