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 PyTorch SparseTensor support for GCNConv and gcn_norm #6033

Merged
merged 13 commits into from
Dec 6, 2022
Merged

Conversation

EdisonLeeeee
Copy link
Contributor

For gcn_norm, a PyTorch SparseTensor is converted to torch_sparse.SparseTensor and then converted back. Not really like this but have no better solution.

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 for this PR. I think this gets really tricky to implement with TorchScript support. Sadly, the full tests do not pass in this PR but I also failed to make them work :(

@EdisonLeeeee
Copy link
Contributor Author

Yeah, I have been blocked by this problem and also failed to find a solution these days. It seems we should drop the TorchScript support for PyTorch SparseTensor for now.

@rusty1s
Copy link
Member

rusty1s commented Nov 23, 2022

Yeah, we don't have support for this anyway yet. The challenge is to ensure that original TorchScript support is maintained :(

@EdisonLeeeee
Copy link
Contributor Author

Noticed now. The problem seems related to is_torch_sparse_tensor - unable to distinguish between the Tensor(dense) and torch.sparse.Tensor when converting to torchscript :(

@codecov
Copy link

codecov bot commented Dec 1, 2022

Codecov Report

Merging #6033 (f08b95b) into master (11e576b) will increase coverage by 0.00%.
The diff coverage is 100.00%.

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

@@           Coverage Diff           @@
##           master    #6033   +/-   ##
=======================================
  Coverage   84.35%   84.36%           
=======================================
  Files         365      365           
  Lines       20500    20512   +12     
=======================================
+ Hits        17293    17305   +12     
  Misses       3207     3207           
Impacted Files Coverage Δ
torch_geometric/nn/conv/gcn_conv.py 98.13% <100.00%> (+0.19%) ⬆️
torch_geometric/utils/softmax.py 100.00% <0.00%> (ø)
torch_geometric/nn/norm/layer_norm.py 100.00% <0.00%> (ø)
torch_geometric/nn/conv/rgcn_conv.py 95.39% <0.00%> (+0.09%) ⬆️
torch_geometric/nn/dense/linear.py 87.40% <0.00%> (+0.18%) ⬆️
torch_geometric/sampler/neighbor_sampler.py 67.05% <0.00%> (+0.25%) ⬆️

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

@EdisonLeeeee
Copy link
Contributor Author

@rusty1s I've made the tests passed with some changes on gcn_norm when dealing with torch.sparse.Tensor, it is somehow tricky though.

@EdisonLeeeee EdisonLeeeee marked this pull request as ready for review December 1, 2022 14:58
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.

This is quite clever. Thank you!

@rusty1s rusty1s enabled auto-merge (squash) December 6, 2022 12:38
@rusty1s rusty1s merged commit 82d8ad2 into master Dec 6, 2022
@rusty1s rusty1s deleted the gcn branch December 6, 2022 12:42
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