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

Respect flow argument in GCN normalization #5149

Merged
merged 8 commits into from
Aug 8, 2022
Merged

Respect flow argument in GCN normalization #5149

merged 8 commits into from
Aug 8, 2022

Conversation

gau-nernst
Copy link
Contributor

@gau-nernst gau-nernst commented Aug 6, 2022

Add flow to gcn_norm to correctly normalize adjacency matrix when flow=target_to_source for directed graphs. This is discussed in #5100.

A simple test is added to verify this new behavior.

@rusty1s Let me know if you need me to edit any thing.

@codecov
Copy link

codecov bot commented Aug 6, 2022

Codecov Report

Merging #5149 (4d9f4cb) into master (8f27635) will increase coverage by 0.00%.
The diff coverage is 100.00%.

❗ Current head 4d9f4cb differs from pull request most recent head c5be455. Consider uploading reports for the commit c5be455 to get more accurate results

@@           Coverage Diff           @@
##           master    #5149   +/-   ##
=======================================
  Coverage   82.98%   82.98%           
=======================================
  Files         333      333           
  Lines       18365    18367    +2     
=======================================
+ Hits        15240    15242    +2     
  Misses       3125     3125           
Impacted Files Coverage Δ
torch_geometric/nn/conv/gcn_conv.py 97.89% <100.00%> (+0.04%) ⬆️

📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more

@rusty1s rusty1s assigned rusty1s and gau-nernst and unassigned rusty1s Aug 6, 2022
@rusty1s rusty1s changed the title Add flow to gcn norm Respect flow argument in GCN normalization Aug 6, 2022
Copy link
Member

@wsad1 wsad1 left a comment

Choose a reason for hiding this comment

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

Thanks for the fix.

gau-nernst and others added 2 commits August 8, 2022 15:55
Co-authored-by: Jinu Sunil <jinu.sunil@gmail.com>
@gau-nernst
Copy link
Contributor Author

@wsad1 I have updated the changelog and merged from master. 1 unrelated test is failing (test_copy_linear[True]), probably due to nan values.

@rusty1s
Copy link
Member

rusty1s commented Aug 8, 2022

Thanks @gau-nernst. Also added the flow argument to other layers that utilize gcn_norm :)

@rusty1s rusty1s enabled auto-merge (squash) August 8, 2022 13:03
@rusty1s rusty1s merged commit 7692969 into pyg-team:master Aug 8, 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.

3 participants