Skip to content

Conversation

@EdisonLeeeee
Copy link
Contributor

@EdisonLeeeee EdisonLeeeee commented Mar 3, 2023

This PR aims to add PyTorch Sparse Tensor support for remove_self_loops, add_self_loops, and maybe_num_nodes, which will be used for supporting PyTorch Sparse Tensor in torch_geometric.nn.conv, e.g. AGNNConv:

if self.add_self_loops:
if isinstance(edge_index, Tensor):
edge_index, _ = remove_self_loops(edge_index)
edge_index, _ = add_self_loops(edge_index,
num_nodes=x.size(self.node_dim))

There are some questions to be clarified:

  • Should I also add support for other functions like add_remaining_self_loops?
  • Should I also update the doc-strings correspondingly?

@codecov
Copy link

codecov bot commented Mar 3, 2023

Codecov Report

Merging #6847 (935be98) into master (5247917) will decrease coverage by 0.24%.
The diff coverage is 100.00%.

❗ Current head 935be98 differs from pull request most recent head 985bf6b. Consider uploading reports for the commit 985bf6b to get more accurate results

@@            Coverage Diff             @@
##           master    #6847      +/-   ##
==========================================
- Coverage   91.74%   91.50%   -0.24%     
==========================================
  Files         427      427              
  Lines       23173    23076      -97     
==========================================
- Hits        21259    21116     -143     
- Misses       1914     1960      +46     
Impacted Files Coverage Δ
torch_geometric/utils/loop.py 100.00% <100.00%> (ø)
torch_geometric/utils/num_nodes.py 96.77% <100.00%> (+0.34%) ⬆️
torch_geometric/visualization/graph.py 68.25% <0.00%> (-26.99%) ⬇️
torch_geometric/nn/conv/utils/typing.py 83.75% <0.00%> (-15.00%) ⬇️
torch_geometric/nn/pool/asap.py 91.89% <0.00%> (-8.11%) ⬇️
torch_geometric/nn/models/attentive_fp.py 95.83% <0.00%> (-4.17%) ⬇️
torch_geometric/io/tu.py 90.80% <0.00%> (-2.30%) ⬇️
torch_geometric/transforms/virtual_node.py 93.47% <0.00%> (-2.18%) ⬇️
torch_geometric/nn/models/basic_gnn.py 89.26% <0.00%> (-1.70%) ⬇️
torch_geometric/nn/glob.py 76.92% <0.00%> (-1.65%) ⬇️
... and 30 more

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

assert out[0].tolist() == expected
assert out[1].tolist() == [[1, 2], [3, 4]]

adj = to_torch_coo_tensor(edge_index)
Copy link
Member

Choose a reason for hiding this comment

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

to_troch_coo_tensor returns a sparse torch.Tensor. It would be nice to add a test for SparseTensor too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you mean to add tests with torch_sparse.SparseTensor as input for these functions as well?

Copy link
Member

Choose a reason for hiding this comment

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

Actually skip that. I guess we only want to support sparse torch.Tensor here, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, exactly.

@wsad1 wsad1 enabled auto-merge (squash) March 7, 2023 08:37
@wsad1 wsad1 merged commit c029841 into master Mar 7, 2023
@wsad1 wsad1 deleted the loop branch March 7, 2023 08:44
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