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.

Sign up for GitHub

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

Jump to bottom

Add combine support to MultiAggregation #5000

Merged
merged 29 commits into from
Jul 21, 2022
Merged

Add combine support to MultiAggregation #5000

merged 29 commits into from
Jul 21, 2022

Conversation

lightaime
Copy link
Contributor

This pull request added the combine support to MultiAggregation.

Addresses #4712

@lightaime lightaime self-assigned this Jul 17, 2022
Guohao Li added 2 commits July 17, 2022 20:13
@lightaime lightaime mentioned this pull request Jul 17, 2022
Closed
26 tasks
@codecov
Copy link

codecov bot commented Jul 18, 2022 •
edited
Loading

Codecov Report

Merging #5000 (b17a869) into master (11f16e3) will increase coverage by 0.03%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #5000      +/-   ##
==========================================
+ Coverage   82.82%   82.85%   +0.03%     
==========================================
  Files         331      331              
  Lines       18069    18107      +38     
==========================================
+ Hits        14965    15003      +38     
  Misses       3104     3104
Impacted Files Coverage Δ
torch_geometric/nn/aggr/multi.py 98.33% <100.00%> (+2.87%) ⬆️
torch_geometric/nn/conv/message_passing.py 98.84% <100.00%> (ø)

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

Guohao Li added 2 commits July 18, 2022 19:55
@lightaime lightaime changed the title [WIP] Add combine support to MultiAggregation Add combine support to MultiAggregation Jul 18, 2022
@lightaime lightaime requested review from rusty1s and Padarn July 18, 2022 20:08
Padarn
Padarn approved these changes Jul 19, 2022
View reviewed changes
Copy link
Contributor

@Padarn Padarn left a comment

Choose a reason for hiding this comment

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

Looks good to me. Left a few minor comments

@lightaime lightaime mentioned this pull request Jul 19, 2022
Closed
@lightaime lightaime requested a review from Padarn July 19, 2022 20:06
@Padarn
Copy link
Contributor

Padarn commented Jul 19, 2022

Nice. LGTM!

rusty1s
rusty1s reviewed Jul 19, 2022
View reviewed changes
torch_geometric/nn/aggr/multi.py Show resolved Hide resolved
torch_geometric/nn/aggr/multi.py Outdated Show resolved Hide resolved
torch_geometric/nn/aggr/multi.py Outdated Show resolved Hide resolved
torch_geometric/nn/aggr/multi.py Outdated Show resolved Hide resolved
torch_geometric/nn/aggr/multi.py Outdated Show resolved Hide resolved
torch_geometric/nn/aggr/multi.py Outdated Show resolved Hide resolved
torch_geometric/nn/aggr/multi.py Outdated Show resolved Hide resolved
torch_geometric/nn/aggr/multi.py Outdated Show resolved Hide resolved
torch_geometric/nn/aggr/multi.py Outdated Show resolved Hide resolved
torch_geometric/nn/aggr/multi.py Outdated Show resolved Hide resolved
rusty1s
rusty1s approved these changes Jul 21, 2022
View reviewed changes
test/nn/conv/test_message_passing.py Outdated Show resolved Hide resolved
test/nn/aggr/test_multi.py Show resolved Hide resolved
torch_geometric/nn/aggr/multi.py Outdated Show resolved Hide resolved
torch_geometric/nn/aggr/multi.py Outdated Show resolved Hide resolved
torch_geometric/nn/aggr/multi.py Outdated Show resolved Hide resolved
torch_geometric/nn/conv/message_passing.py Show resolved Hide resolved
torch_geometric/nn/conv/message_passing.py Outdated Show resolved Hide resolved
torch_geometric/nn/aggr/multi.py Outdated Show resolved Hide resolved
torch_geometric/nn/aggr/multi.py Outdated Show resolved Hide resolved
torch_geometric/nn/aggr/multi.py Outdated Show resolved Hide resolved
rusty1s
rusty1s approved these changes Jul 21, 2022
View reviewed changes
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.

Looks amazing, just some last comments. Please feel free to merge afterwards.

test/nn/conv/test_message_passing.py Outdated Show resolved Hide resolved
test/nn/conv/test_message_passing.py Outdated Show resolved Hide resolved
torch_geometric/nn/aggr/multi.py Show resolved Hide resolved
torch_geometric/nn/aggr/multi.py Outdated Show resolved Hide resolved
torch_geometric/nn/aggr/multi.py Outdated Show resolved Hide resolved
torch_geometric/nn/aggr/multi.py Outdated Show resolved Hide resolved
torch_geometric/nn/aggr/multi.py Outdated Show resolved Hide resolved
@lightaime
Copy link
Contributor Author

Thanks, @rusty1s @Padarn for the helpful comments!

@lightaime lightaime merged commit 02eeea3 into master Jul 21, 2022
@lightaime lightaime deleted the multi_aggr_combine branch July 21, 2022 16:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Padarn Padarn Padarn approved these changes

@rusty1s rusty1s rusty1s approved these changes

Assignees

@lightaime lightaime

Labels
0 - Priority P0 feature nn
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@lightaime @Padarn @rusty1s