Repository: pyg-team/pytorch_geometric
Pull request #8210 (2023-10-17 16:05:15)

Disallow `add_self_loops=True` in `GCNConv` when `normalize=False`
Fix https://github.com/pyg-team/pytorch_geometric/issues/6251

codecov[bot]: ## [Codecov](https://app.codecov.io/gh/pyg-team/pytorch_geometric/pull/8210?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pyg-team) Report
> Merging [#8210](https://app.codecov.io/gh/pyg-team/pytorch_geometric/pull/8210?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pyg-team) (5b37943) into [master](https://app.codecov.io/gh/pyg-team/pytorch_geometric/commit/ec6264bd9cd39f8f47abcad1a899b53fa92ae6d6?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pyg-team) (ec6264b) will **decrease** coverage by `0.03%`.
> The diff coverage is `41.17%`.

> :exclamation: Current head 5b37943 differs from pull request most recent head 5d9c8c1. Consider uploading reports for the commit 5d9c8c1 to get more accurate results

```diff
@@            Coverage Diff             @@
##           master    #8210      +/-   ##
==========================================
- Coverage   87.39%   87.37%   -0.03%     
==========================================
  Files         473      473              
  Lines       28595    28612      +17     
==========================================
+ Hits        24992    24999       +7     
- Misses       3603     3613      +10     
```


| [Files](https://app.codecov.io/gh/pyg-team/pytorch_geometric/pull/8210?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pyg-team) | Coverage Δ | |
|---|---|---|
| [torch\_geometric/nn/conv/gcn\_conv.py](https://app.codecov.io/gh/pyg-team/pytorch_geometric/pull/8210?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pyg-team#diff-dG9yY2hfZ2VvbWV0cmljL25uL2NvbnYvZ2NuX2NvbnYucHk=) | `92.00% <41.17%> (-8.00%)` | :arrow_down: |

:mega: We’re building smart automated test selection to slash your CI/CD build times. [Learn more](https://about.codecov.io/iterative-testing/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pyg-team)

EdisonLeeeee: Thank you for the PR, I don't think it is necessary to make these changes, as 

- `normalize` is the core operation in `GCNConv`, and in most cases, it is recommended to set it to `True` when using `GCNConv` as the convolution layer.
- While there are situations where using `GCNConv` without normalization but with self-loops might be desired, it is possible to manually add self-loops during the forward pass or data preprocessing. In such cases, it might be more appropriate to consider using `SAGEConv`.
- To support such cases, adding the functionality of self-loops without normalization to `GCNConv` would require introducing additional code, which could potentially make the implementation more complex and harder to read.

Perhaps we can simply add a warning when `normalize=False` but `add_self_loops=True`, WDYT?
xnuohz: > Thank you for the PR, I don't think it is necessary to make these changes, as
> 
> * `normalize` is the core operation in `GCNConv`, and in most cases, it is recommended to set it to `True` when using `GCNConv` as the convolution layer.
> * While there are situations where using `GCNConv` without normalization but with self-loops might be desired, it is possible to manually add self-loops during the forward pass or data preprocessing. In such cases, it might be more appropriate to consider using `SAGEConv`.
> * To support such cases, adding the functionality of self-loops without normalization to `GCNConv` would require introducing additional code, which could potentially make the implementation more complex and harder to read.
> 
> Perhaps we can simply add a warning when `normalize=False` but `add_self_loops=True`, WDYT?

Generally agree. There are two solutions I've considered

- Directly forbid `normalize=False`, indeed in most cases we will not set it like this
- Support `normalize=False` to make sure the return is correct, since currently the results are incorrect when `add_self_loops=True`.

And I chose the 2nd one, haha.
xnuohz: > Thank you. This is actually a breaking change which we would need to avoid. As such, can we make `add_self_loops` be of type `Optional[bool] = None`, and set it based on `normalize=False/True` whenever it is not explicitly passed?

How about `add_self_loops=False` for `normalize=False` and `add_self_loops=True` for `normalize=True` ? If so, then a warning is needed when we explicitly pass `normalize=False` and `add_self_loops=True`.
rusty1s: I just read @EdisonLeeeee's post, and need to agree with this. Normalization in `GCNConv` code is quite complex and we shouldn't move away from the original implementation too much.

Patch:
diff --git a/CHANGELOG.md b/CHANGELOG.md
index 445622b45..02fbf5009 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -13,6 +13,7 @@ The format is based on [Keep a Changelog](http://keepachangelog.com/en/1.0.0/).
 
 ### Changed
 
+- Disallow the usage of `add_self_loops=True` in `GCNConv(normalize=False)` ([#8210](https://github.com/pyg-team/pytorch_geometric/pull/8210))
 - Disable device asserts during `torch_geometric.compile` ([#8220](https://github.com/pyg-team/pytorch_geometric/pull/8220))
 
 ### Deprecated
diff --git a/torch_geometric/nn/conv/gcn_conv.py b/torch_geometric/nn/conv/gcn_conv.py
index 4efc83902..e1d330430 100644
--- a/torch_geometric/nn/conv/gcn_conv.py
+++ b/torch_geometric/nn/conv/gcn_conv.py
@@ -145,9 +145,11 @@ class GCNConv(MessagePassing):
             This parameter should only be set to :obj:`True` in transductive
             learning scenarios. (default: :obj:`False`)
         add_self_loops (bool, optional): If set to :obj:`False`, will not add
-            self-loops to the input graph. (default: :obj:`True`)
+            self-loops to the input graph. By default, self-loops will be added
+            in case :obj:`normalize` is set to :obj:`True`, and not added
+            otherwise. (default: :obj:`None`)
         normalize (bool, optional): Whether to add self-loops and compute
-            symmetric normalization coefficients on the fly.
+            symmetric normalization coefficients on-the-fly.
             (default: :obj:`True`)
         bias (bool, optional): If set to :obj:`False`, the layer will not learn
             an additive bias. (default: :obj:`True`)
@@ -170,7 +172,7 @@ class GCNConv(MessagePassing):
         out_channels: int,
         improved: bool = False,
         cached: bool = False,
-        add_self_loops: bool = True,
+        add_self_loops: Optional[bool] = None,
         normalize: bool = True,
         bias: bool = True,
         **kwargs,
@@ -178,6 +180,14 @@ class GCNConv(MessagePassing):
         kwargs.setdefault('aggr', 'add')
         super().__init__(**kwargs)
 
+        if add_self_loops is None:
+            add_self_loops = normalize
+
+        if add_self_loops and not normalize:
+            raise ValueError(f"'{self.__class__.__name__}' does not support "
+                             f"adding self-loops to the graph when no "
+                             f"on-the-fly normalization is applied")
+
         self.in_channels = in_channels
         self.out_channels = out_channels
         self.improved = improved


Test patch:
diff --git a/test/nn/conv/test_gcn_conv.py b/test/nn/conv/test_gcn_conv.py
index 63ba7da52..e96165a23 100644
--- a/test/nn/conv/test_gcn_conv.py
+++ b/test/nn/conv/test_gcn_conv.py
@@ -99,10 +99,14 @@ def test_static_gcn_conv():
     assert out.size() == (3, 4, 32)
 
 
-def test_gcn_conv_norm():
+def test_gcn_conv_error():
+    with pytest.raises(ValueError, match="does not support adding self-loops"):
+        GCNConv(16, 32, normalize=False, add_self_loops=True)
+
+
+def test_gcn_conv_flow():
     x = torch.randn(4, 16)
     edge_index = torch.tensor([[0, 0, 0], [1, 2, 3]])
-    row, col = edge_index
 
     conv = GCNConv(16, 32, flow="source_to_target")
     out1 = conv(x, edge_index)