Deprecate `nn.glob` package #5039

Padarn · 2022-07-23T07:02:25Z

Removes use of nn.glob package. #4712

codecov · 2022-07-23T07:06:25Z

Codecov Report

Merging #5039 (0357f32) into master (575611f) will decrease coverage by 1.84%.
The diff coverage is 92.85%.

@@            Coverage Diff             @@
##           master    #5039      +/-   ##
==========================================
- Coverage   84.74%   82.89%   -1.85%     
==========================================
  Files         331      331              
  Lines       18162    18137      -25     
==========================================
- Hits        15391    15035     -356     
- Misses       2771     3102     +331

Impacted Files	Coverage Δ
torch_geometric/nn/glob.py	`81.25% <90.00%> (ø)`
torch_geometric/nn/__init__.py	`100.00% <100.00%> (ø)`
torch_geometric/nn/pool/__init__.py	`89.65% <100.00%> (+0.36%)`	⬆️
torch_geometric/nn/pool/glob.py	`100.00% <100.00%> (ø)`
torch_geometric/nn/models/dimenet_utils.py	`0.00% <0.00%> (-75.52%)`	⬇️
torch_geometric/nn/models/dimenet.py	`14.51% <0.00%> (-53.00%)`	⬇️
torch_geometric/nn/conv/utils/typing.py	`81.25% <0.00%> (-17.50%)`	⬇️
torch_geometric/profile/profile.py	`32.94% <0.00%> (-15.30%)`	⬇️
torch_geometric/nn/inits.py	`67.85% <0.00%> (-7.15%)`	⬇️
torch_geometric/nn/resolver.py	`88.00% <0.00%> (-6.00%)`	⬇️
... and 12 more

Help us with your feedback. Take ten seconds to tell us how you rate us.

CHANGELOG.md

lightaime

Thanks! LGTM! Left a few minor comments.

lightaime · 2022-07-23T14:41:24Z

torch_geometric/nn/pool/glob.py

+sum_aggr = SumAggregation()
+
+
+def global_add_pool(x: Tensor, index: Optional[Tensor],


Should we keep the original doc strings for global pools?

lightaime · 2022-07-23T14:48:10Z

torch_geometric/nn/pool/glob.py

+
+@deprecated(
+    details="use 'nn.aggr.GlobalSortAggr' instead",
+    func_name='nn.glob.global_sort_pool',


Here is a bit confusing for me since the modules are now moved to nn.pool.glob.*.

Thanks good point!

torch_geometric/nn/pool/glob.py

lightaime · 2022-07-23T15:05:22Z

torch_geometric/nn/glob/glob.py

-    return scatter(x, batch, dim=-2, dim_size=size, reduce='max')
-
-
-class GlobalPooling(torch.nn.Module):


Could you please clarify why this is removed completely? If we need it can we implement it with MultiAggregation?

Yes removed completed (discussed with @rusty1s in earlier version of this PR - the code for this was added recently and we decided it was no longer needed)

lightaime · 2022-07-23T15:06:33Z

test/nn/aggr/test_sort.py

@@ -1,78 +0,0 @@
-import torch


Could you please clarify why the tests are removed?

Mistake, added back.

Co-authored-by: Guohao Li <lightaime@gmail.com>

Padarn · 2022-07-24T00:26:21Z

Thanks for the review @lightaime

rusty1s

Thanks @Padarn. I think my comments earlier let to some misunderstanding, sorry about that. Clarified more in the comments. Let me know what you think!

test/nn/aggr/test_sort.py

rusty1s · 2022-07-24T04:04:34Z

test/nn/glob/test_glob.py

@@ -1,72 +0,0 @@
-import torch


Move this file to pool/test_glob.py?

rusty1s · 2022-07-24T04:07:28Z

torch_geometric/nn/pool/__init__.py

@@ -11,6 +11,8 @@
 from .asap import ASAPooling
 from .pan_pool import PANPooling
 from .mem_pool import MemPooling
+from .glob import (global_max_pool, global_mean_pool, global_add_pool,


Ah sorry, this is not what I meant. I meant:

Move global_add_pool, global_mean_pool, and global_max_pool to glob/glob.py to pool/glob.py`.

Move glob/__init__.py to nn/glob.py and keep the deprecations in there.

The idea here is that we keep the implementations of global_*_pool as they were (since they are so heavily used).

hmm sorry I don't fully understand the distinction between these. In both cases you can

from torch_geometric.nn import global_max_pool

is it that you want to keep this?

from torch_geometric.nn.glob import global_max_pool

Or you just want to clearly separate the deprecations?

Yes, we only need to
deprecate the import of torch_geometric.nn.glob.

Padarn · 2022-07-24T04:25:06Z

Ah yeah I misunderstood. Thanks for clarifying, I'll make the changes

lightaime

LGTM! Left some small comments.

lightaime · 2022-07-25T15:56:51Z

torch_geometric/nn/pool/__init__.py

+    'MemPooling', 'max_pool', 'avg_pool', 'max_pool_x', 'max_pool_neighbor_x',
+    'avg_pool_x', 'avg_pool_neighbor_x', 'graclus', 'voxel_grid', 'fps', 'knn',
+    'knn_graph', 'radius', 'radius_graph', 'nearest', 'global_max_pool',
+    'global_add_pool', 'global_mean_pool'


Should we add back a comma for auto-formatting?

+1. Let‘s also move the global pooling methods to the top.

lightaime · 2022-07-25T16:15:44Z

torch_geometric/nn/glob.py

+
+Set2Set = deprecated(
+    details="use 'nn.aggr.Set2Set' instead",
+    func_name='nn.pool.Set2Set',


The deprecations are moved back to nn.glob.*. Should we change back from nn.pool.* to nn.glob.*? Sorry about that.

I think this is correct. We provide backward compatibility in nn/glob.py but deprecate its use.

rusty1s

Thank you! This is exactly what I had in mind. Left some last comments.

rusty1s · 2022-07-25T20:31:39Z

torch_geometric/nn/glob.py

+
+Set2Set = deprecated(
+    details="use 'nn.aggr.Set2Set' instead",
+    func_name='nn.pool.Set2Set',


I think this is correct. We provide backward compatibility in nn/glob.py but deprecate its use.

rusty1s · 2022-07-25T20:32:06Z

torch_geometric/nn/glob.py

@@ -0,0 +1,35 @@
+from torch_geometric.deprecation import deprecated


The global_*_pool methods need to be added here as well.

rusty1s · 2022-07-25T20:32:25Z

torch_geometric/nn/pool/__init__.py

+    'MemPooling', 'max_pool', 'avg_pool', 'max_pool_x', 'max_pool_neighbor_x',
+    'avg_pool_x', 'avg_pool_neighbor_x', 'graclus', 'voxel_grid', 'fps', 'knn',
+    'knn_graph', 'radius', 'radius_graph', 'nearest', 'global_max_pool',
+    'global_add_pool', 'global_mean_pool'


+1. Let‘s also move the global pooling methods to the top.

rusty1s · 2022-07-25T20:34:01Z

torch_geometric/nn/pool/glob.py

@@ -25,19 +29,19 @@ def global_add_pool(x: Tensor, batch: Optional[Tensor],
    """
    if batch is None:
        return x.sum(dim=-2, keepdim=x.dim() == 2)
-    size = int(batch.max().item() + 1) if size is None else size
-    return scatter(x, batch, dim=-2, dim_size=size, reduce='add')
+    return sum_aggr(x, batch, dim_size=size)


Can we leave the implementation as it is? I am not super happy with having global modules here.

Padarn · 2022-07-26T00:13:08Z

Thanks for the reviews again! I've addressed the comments. Will merge later unless any objections

Padarn · 2022-07-26T04:38:39Z

I have some problem with the docs build. Will figure it out tonight

Padarn · 2022-07-26T11:03:40Z

could be sphinx-doc/sphinx#10705 .. may have to wait until they release the new version

rusty1s · 2022-07-26T11:09:29Z

Yes, don‘t worry about it.

Padarn added 5 commits July 23, 2022 14:52

add only needed

814bb12

delete test

b281587

delete test

eada40a

revert name change

d8b0218

update changelog

72edcab

remove doc references

f1c453a

Padarn requested review from rusty1s and lightaime July 23, 2022 07:10

lightaime reviewed Jul 23, 2022

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

lightaime assigned Padarn Jul 23, 2022

lightaime added 0 - Priority P0 refactor nn labels Jul 23, 2022

lightaime reviewed Jul 23, 2022

View reviewed changes

Padarn and others added 3 commits July 24, 2022 08:25

Update torch_geometric/nn/pool/glob.py

01a6482

Co-authored-by: Guohao Li <lightaime@gmail.com>

Update torch_geometric/nn/pool/glob.py

9fd15d4

Co-authored-by: Guohao Li <lightaime@gmail.com>

Update CHANGELOG.md

99b9d5e

Co-authored-by: Guohao Li <lightaime@gmail.com>

Padarn added 2 commits July 24, 2022 08:29

readd docstring

4e04bcd

change depreciaton notice

44fb56e

rusty1s reviewed Jul 24, 2022

View reviewed changes

add back dense methods

fdf7379

lightaime approved these changes Jul 25, 2022

View reviewed changes

rusty1s approved these changes Jul 25, 2022

View reviewed changes

Padarn added 2 commits July 26, 2022 08:11

formatting and naming

108abff

Merge branch 'master' into padarn/depreciate-glob

f8bc2d7

fix import order

702c00e

fix implementation of max

0357f32

rusty1s merged commit 9ecd507 into pyg-team:master Jul 26, 2022

lightaime mentioned this pull request Jul 26, 2022

[Roadmap] torch_geometric.nn.aggr 🚀 #4712

Closed

26 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Deprecate `nn.glob` package #5039

Deprecate `nn.glob` package #5039

Padarn commented Jul 23, 2022 •

edited

Loading

codecov bot commented Jul 23, 2022 •

edited

Loading

lightaime left a comment •

edited

Loading

lightaime Jul 23, 2022

Padarn Jul 24, 2022

lightaime Jul 23, 2022

Padarn Jul 24, 2022

lightaime Jul 23, 2022 •

edited

Loading

Padarn Jul 24, 2022

lightaime Jul 25, 2022

lightaime Jul 23, 2022

Padarn Jul 24, 2022

Padarn commented Jul 24, 2022

rusty1s left a comment •

edited

Loading

rusty1s Jul 24, 2022 •

edited

Loading

rusty1s Jul 24, 2022

rusty1s Jul 24, 2022

Padarn Jul 24, 2022

Padarn Jul 24, 2022

rusty1s Jul 24, 2022

Padarn commented Jul 24, 2022

lightaime left a comment

lightaime Jul 25, 2022 •

edited

Loading

rusty1s Jul 25, 2022

lightaime Jul 25, 2022

rusty1s Jul 25, 2022

rusty1s left a comment

rusty1s Jul 25, 2022

rusty1s Jul 25, 2022

rusty1s Jul 25, 2022

rusty1s Jul 25, 2022

Padarn commented Jul 26, 2022

Padarn commented Jul 26, 2022

Padarn commented Jul 26, 2022

rusty1s commented Jul 26, 2022

		sum_aggr = SumAggregation()


		def global_add_pool(x: Tensor, index: Optional[Tensor],

		return scatter(x, batch, dim=-2, dim_size=size, reduce='max')


		class GlobalPooling(torch.nn.Module):

		@@ -0,0 +1,35 @@
		from torch_geometric.deprecation import deprecated

Deprecate nn.glob package #5039

Deprecate nn.glob package #5039

Conversation

Padarn commented Jul 23, 2022 • edited Loading

codecov bot commented Jul 23, 2022 • edited Loading

Codecov Report

lightaime left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lightaime Jul 23, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Padarn commented Jul 24, 2022

rusty1s left a comment • edited Loading

Choose a reason for hiding this comment

rusty1s Jul 24, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Padarn commented Jul 24, 2022

lightaime left a comment

Choose a reason for hiding this comment

lightaime Jul 25, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rusty1s left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Padarn commented Jul 26, 2022

Padarn commented Jul 26, 2022

Padarn commented Jul 26, 2022

rusty1s commented Jul 26, 2022

Deprecate `nn.glob` package #5039

Deprecate `nn.glob` package #5039

Padarn commented Jul 23, 2022 •

edited

Loading

codecov bot commented Jul 23, 2022 •

edited

Loading

lightaime left a comment •

edited

Loading

lightaime Jul 23, 2022 •

edited

Loading

rusty1s left a comment •

edited

Loading

rusty1s Jul 24, 2022 •

edited

Loading

lightaime Jul 25, 2022 •

edited

Loading