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

Fix bugs in PNAConv and DegreeScalerAggregation #6609

Merged
merged 5 commits into from
Feb 6, 2023

Conversation

zechengz
Copy link
Member

@zechengz zechengz commented Feb 6, 2023

Some fixes, including bugs fixes and minor documentation fixes

torch_geometric/nn/aggr/scaler.py

The deg tensor should be clamped during the inverse_linear computation and thus avoid division by zero (in case the deg tensor contains a zero degree value). Previous implementation will make the zero degree value equal to two in the amplification and attenuation cases where we also add one directly. Though I am not sure whether this way is correct and whether we also need to clamp(1) in the linear scaler.

torch_geometric/nn/conv/pna_conv.py

The previous implementation utilized two for loops to compute the deg_histogram tensor. The first loop calculates the max_degree, while the second loop updates the deg_histogram by adding each bincount, with a minlength of max_degree + 1. However, for certain loaders (e.g. the NeighborLoader), the degree in the second loop may exceed the max_degree computed in the first loop and thus raise following error. Also, current implementation that replaces the two loops with one loop may potentially improve computation speed, although I don't verify this...

    deg_histogram += torch.bincount(d, minlength=deg_histogram.numel())
RuntimeError: The size of tensor a (17) must match the size of tensor b (19) at non-singleton dimension 0

@zechengz zechengz marked this pull request as ready for review February 6, 2023 10:02
@codecov
Copy link

codecov bot commented Feb 6, 2023

Codecov Report

Merging #6609 (a930b6d) into master (a930b6d) will not change coverage.
The diff coverage is n/a.

❗ Current head a930b6d differs from pull request most recent head 90f3da5. Consider uploading reports for the commit 90f3da5 to get more accurate results

@@           Coverage Diff           @@
##           master    #6609   +/-   ##
=======================================
  Coverage   87.40%   87.40%           
=======================================
  Files         421      421           
  Lines       22903    22903           
=======================================
  Hits        20018    20018           
  Misses       2885     2885           

📣 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.

Thanks!

@rusty1s rusty1s enabled auto-merge (squash) February 6, 2023 11:01
@rusty1s rusty1s merged commit d1ad8c7 into pyg-team:master Feb 6, 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.

None yet

2 participants