Refactor `PNAConv` to rely on new `Aggregation` #4864

Padarn · 2022-06-26T01:44:28Z

Simply remove the unneeded handling of aggregation in PNAConv now that this s handled in MessagePassing.

Noted in #4712

codecov · 2022-06-26T01:47:37Z

Codecov Report

Merging #4864 (2c7a289) into master (e430d94) will decrease coverage by 1.86%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #4864      +/-   ##
==========================================
- Coverage   84.58%   82.72%   -1.87%     
==========================================
  Files         329      330       +1     
  Lines       17847    17844       -3     
==========================================
- Hits        15096    14761     -335     
- Misses       2751     3083     +332

Impacted Files	Coverage Δ
torch_geometric/nn/aggr/__init__.py	`100.00% <100.00%> (ø)`
torch_geometric/nn/aggr/scaler.py	`100.00% <100.00%> (ø)`
torch_geometric/nn/conv/pna_conv.py	`92.00% <100.00%> (-2.65%)`	⬇️
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/nn/inits.py	`67.85% <0.00%> (-7.15%)`	⬇️
torch_geometric/nn/resolver.py	`86.04% <0.00%> (-6.98%)`	⬇️
torch_geometric/io/tu.py	`93.90% <0.00%> (-2.44%)`	⬇️
torch_geometric/nn/models/mlp.py	`98.52% <0.00%> (-1.48%)`	⬇️
... and 6 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e430d94...2c7a289. Read the comment docs.

rusty1s

Thanks! This looks pretty clean now :)

rusty1s · 2022-06-26T09:14:50Z

torch_geometric/nn/conv/pna_conv.py

-                raise ValueError(f'Unknown aggregator "{aggregator}".')
-            outs.append(out)
-        out = torch.cat(outs, dim=-1)
+        out = super().aggregate(inputs, index, dim_size=dim_size)

        deg = degree(index, dim_size, dtype=inputs.dtype)


Can we move these scalers outside aggregate? This way, we don't have to override aggregate at all.

Oh good idea :-)

I updated to apply the scalers as a hook - WDYT?

Interesting solution, although I personally feel we shouldn't add business logic to hooks TBH. If you are not a fan of moving this logic post self.propagate, we could add it as an additional module to nn.aggr, e.g.,

class DegreeScalerAggregation(Aggregation): def __init__(self, aggregator, scalers, deg): pass

Alternatively, I may be mistaken to move the logic outside aggregate - This should definitely be part of aggregation.

Actually I like the degree scaler as an Aggregation. It keeps it within the Aggregation logic and should be eaiser to reuse.

Agree on the hooks not feeling like a great solution.

Agree with adding the degree scaler as an Aggregation is better. Alternatively, we can add it as a helper method to the Aggregation base class instead of an Aggregation.

I've made changes for this now. I'm leaving some comments in the diff though where I'd like some input

rusty1s · 2022-06-29T05:49:03Z

@Padarn Let me know if you need any help in adding DegreeScalerAggregation :)

Padarn · 2022-06-29T05:57:58Z

Hey, I'm happy to do this but not a lot of time during the week. If you want to get this merged sooner please don't hesitate to take over.

…

On Wed, 29 Jun 2022, 1:49 pm Matthias Fey, ***@***.***> wrote: @Padarn <https://github.com/Padarn> Let me know if you need any help in adding DegreeScalerAggregation :) — Reply to this email directly, view it on GitHub <#4864 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAGRPN3SAISTXIWBFCT5KHLVRPPVTANCNFSM5Z3EUHQQ> . You are receiving this because you were mentioned.Message ID: ***@***.***>

-- By communicating with Grab Inc and/or its subsidiaries, associate companies and jointly controlled entities (“Grab Group”), you are deemed to have consented to the processing of your personal data as set out in the Privacy Notice which can be viewed at https://grab.com/privacy/ <https://grab.com/privacy/> This email contains confidential information and is only for the intended recipient(s). If you are not the intended recipient(s), please do not disseminate, distribute or copy this email Please notify Grab Group immediately if you have received this by mistake and delete this email from your system. Email transmission cannot be guaranteed to be secure or error-free as any information therein could be intercepted, corrupted, lost, destroyed, delayed or incomplete, or contain viruses. Grab Group do not accept liability for any errors or omissions in the contents of this email arises as a result of email transmission. All intellectual property rights in this email and attachments therein shall remain vested in Grab Group, unless otherwise provided by law.

Padarn · 2022-06-30T13:18:10Z

I've added the DegreeScalerAggregation - it's a tiny bit ugly right now (just a lift and shift). Also the only tests are through PNAConv.

torch_geometric/nn/aggr/scaler.py

Padarn · 2022-07-02T09:22:47Z

torch_geometric/nn/aggr/scaler.py

+
+        out = self.agg(x, index, ptr, dim_size, dim)
+        deg = degree(index, dtype=out.dtype)
+        deg = deg.clamp_(1).view(*([-1] + [1] * (len(out.shape) - 1)))


This was modified - before it was simply

deg.clamp_(1).view(-1, 1, 1)

I don't know if there is a much cleaner way to achieve the same thing here.

Thanks for fixing. I think this is still not completely right though. How about we take out.size() and write -1 to the dimension given to dim, and use that for reshaping.

Hmm makes sense to use the dimension give to dim, but just to clarify on using out.size(), if for example

out.size() = [4, 4, 96] deg.size() = [4] dim = -2

I assume we want to do

deg.view(1, -1, 1)

so that the scaler can broadcast

Does that make sense?

Yes, that‘s what I meant. Sorry for the confusion.

torch_geometric/nn/aggr/scaler.py

lightaime · 2022-07-02T20:40:58Z

I've added the DegreeScalerAggregation - it's a tiny bit ugly right now (just a lift and shift). Also the only tests are through PNAConv.

@Padarn Thanks for adding the DegreeScalerAggregation. IMO, the degree scaler is more like an optional logic that we can do after reduce instead of an aggregation function. I think it makes more sense to make it a helper function in the base class. What do you think?

Padarn · 2022-07-02T22:09:12Z

Actually I do mostly agree about that, but it has two downsides 1. PNAConv needs to override the aggregation function of MessagePassing 2. The degree tensor needs to be provided to the helper (unless we add this to the constructor of the base class). I'm still okay to go with that suggestion though, WDYT?

…

On Sun, 3 Jul 2022, 4:41 am Guohao Li, ***@***.***> wrote: I've added the DegreeScalerAggregation - it's a tiny bit ugly right now (just a lift and shift). Also the only tests are through PNAConv. @Padarn <https://github.com/Padarn> Thanks for adding the DegreeScalerAggregation. IMO, the degree scaler is more like an optional logic that we can do after reduce instead of an aggregation function. I think it makes more sense to make it a helper function in the base class. What do you think? — Reply to this email directly, view it on GitHub <#4864 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAGRPN3C3HMD7LKS5EAZ5JTVSCSONANCNFSM5Z3EUHQQ> . You are receiving this because you were mentioned.Message ID: ***@***.***>

-- By communicating with Grab Inc and/or its subsidiaries, associate companies and jointly controlled entities (“Grab Group”), you are deemed to have consented to the processing of your personal data as set out in the Privacy Notice which can be viewed at https://grab.com/privacy/ <https://grab.com/privacy/> This email contains confidential information and is only for the intended recipient(s). If you are not the intended recipient(s), please do not disseminate, distribute or copy this email Please notify Grab Group immediately if you have received this by mistake and delete this email from your system. Email transmission cannot be guaranteed to be secure or error-free as any information therein could be intercepted, corrupted, lost, destroyed, delayed or incomplete, or contain viruses. Grab Group do not accept liability for any errors or omissions in the contents of this email arises as a result of email transmission. All intellectual property rights in this email and attachments therein shall remain vested in Grab Group, unless otherwise provided by law.

rusty1s · 2022-07-02T22:21:37Z

I personally like the view of seeing PNA as a composition of aggregators and scalers (that‘s how it is defined in the paper), so I am in favor of keeping it that way.

Padarn · 2022-07-03T00:19:27Z

That makes sense to me.

Thoughts on merging this now then and following up on the items I commented on in the diff separately?

rusty1s

Thank you! LGTM. Feel free to merge once the remaining comments are resolved.

torch_geometric/nn/aggr/__init__.py

torch_geometric/nn/aggr/scaler.py

rusty1s · 2022-07-03T05:56:21Z

torch_geometric/nn/aggr/scaler.py

+
+        out = self.agg(x, index, ptr, dim_size, dim)
+        deg = degree(index, dtype=out.dtype)
+        deg = deg.clamp_(1).view(*([-1] + [1] * (len(out.shape) - 1)))


Thanks for fixing. I think this is still not completely right though. How about we take out.size() and write -1 to the dimension given to dim, and use that for reshaping.

torch_geometric/nn/aggr/scaler.py

Padarn requested review from lightaime and rusty1s June 26, 2022 01:45

rusty1s assigned Padarn Jun 26, 2022

rusty1s added 0 - Priority P0 refactor nn labels Jun 26, 2022

rusty1s changed the title ~~Refactor PNAConv to use rely on new Aggregation~~ Refactor PNAConv to rely on new Aggregation Jun 26, 2022

rusty1s approved these changes Jun 26, 2022

View reviewed changes

rusty1s mentioned this pull request Jun 26, 2022

[Roadmap] torch_geometric.nn.aggr 🚀 #4712

Closed

26 tasks

lightaime approved these changes Jun 28, 2022

View reviewed changes

Padarn added 4 commits June 30, 2022 21:15

migrate PNAConv to new aggregator

50a3b96

add changelog

ab65cd2

refactor to use hook for scale

cd18952

create DegreeScalerAggregation

d2f91fb

Padarn force-pushed the padarn/pna-migrate branch from 0ec3e95 to d2f91fb Compare June 30, 2022 13:16

Padarn added 3 commits June 30, 2022 21:19

pep8

be53614

typo

755ebcf

add test

a05475a

Padarn commented Jul 2, 2022

View reviewed changes

torch_geometric/nn/aggr/scaler.py Show resolved Hide resolved

Padarn commented Jul 2, 2022

View reviewed changes

torch_geometric/nn/aggr/scaler.py Show resolved Hide resolved

Padarn commented Jul 2, 2022

View reviewed changes

torch_geometric/nn/aggr/scaler.py Outdated Show resolved Hide resolved

rusty1s approved these changes Jul 3, 2022

View reviewed changes

address comments

eacfff1

Padarn force-pushed the padarn/pna-migrate branch from 1ec28ad to eacfff1 Compare July 3, 2022 07:30

Padarn commented Jul 3, 2022

View reviewed changes

torch_geometric/nn/aggr/scaler.py Show resolved Hide resolved

Merge branch 'master' into padarn/pna-migrate

2c7a289

Padarn merged commit fd944f3 into pyg-team:master Jul 3, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor `PNAConv` to rely on new `Aggregation` #4864

Refactor `PNAConv` to rely on new `Aggregation` #4864

Padarn commented Jun 26, 2022 •

edited

Loading

codecov bot commented Jun 26, 2022 •

edited

Loading

rusty1s left a comment

rusty1s Jun 26, 2022

Padarn Jun 26, 2022

Padarn Jun 26, 2022

rusty1s Jun 26, 2022

rusty1s Jun 26, 2022

Padarn Jun 26, 2022

lightaime Jun 28, 2022

Padarn Jul 2, 2022

rusty1s commented Jun 29, 2022

Padarn commented Jun 29, 2022 via email

Padarn commented Jun 30, 2022

Padarn Jul 2, 2022

rusty1s Jul 3, 2022

Padarn Jul 3, 2022

rusty1s Jul 3, 2022

lightaime commented Jul 2, 2022

Padarn commented Jul 2, 2022 via email

rusty1s commented Jul 2, 2022

Padarn commented Jul 3, 2022

rusty1s left a comment

rusty1s Jul 3, 2022

Refactor PNAConv to rely on new Aggregation #4864

Refactor PNAConv to rely on new Aggregation #4864

Conversation

Padarn commented Jun 26, 2022 • edited Loading

codecov bot commented Jun 26, 2022 • edited Loading

Codecov Report

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

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 commented Jun 29, 2022

Padarn commented Jun 29, 2022 via email

Padarn commented Jun 30, 2022

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 commented Jul 2, 2022

Padarn commented Jul 2, 2022 via email

rusty1s commented Jul 2, 2022

Padarn commented Jul 3, 2022

rusty1s left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Refactor `PNAConv` to rely on new `Aggregation` #4864

Refactor `PNAConv` to rely on new `Aggregation` #4864

Padarn commented Jun 26, 2022 •

edited

Loading

codecov bot commented Jun 26, 2022 •

edited

Loading