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.

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

Implemented the Learnable Communitive Monoid Aggregation #7976

Merged
merged 12 commits into from
Sep 11, 2023

Conversation

ArchieGertsman
Copy link
Contributor

Initial version of the LCM aggregation, requested in issue #7574. A possible feature to incorporate is for forward to additionally compute and return an associativity loss, as described in the paper.

@codecov
Copy link

codecov bot commented Sep 4, 2023

Codecov Report

Merging #7976 (de1c287) into master (0438d3a) will increase coverage by 0.00%.
The diff coverage is 92.10%.

@@           Coverage Diff           @@
##           master    #7976   +/-   ##
=======================================
  Coverage   89.58%   89.58%           
=======================================
  Files         461      462    +1     
  Lines       26980    27018   +38     
=======================================
+ Hits        24169    24205   +36     
- Misses       2811     2813    +2     
Files Changed Coverage Δ
torch_geometric/nn/aggr/lcm.py 91.89% <91.89%> (ø)
torch_geometric/nn/aggr/__init__.py 100.00% <100.00%> (ø)

... and 1 file with indirect coverage changes

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

@puririshi98
Copy link
Contributor

overall LGTM, but lets wait on @wsad1 and/or @EdisonLeeeee reviews

Copy link
Contributor

@EdisonLeeeee EdisonLeeeee left a comment

Choose a reason for hiding this comment

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

Left some minor comments. Thank you :)

ArchieGertsman and others added 7 commits September 7, 2023 21:57
Co-authored-by: Jintang Li <cnljt@outlook.com>
Co-authored-by: Jintang Li <cnljt@outlook.com>
Co-authored-by: Jintang Li <cnljt@outlook.com>
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.

Wow, this is pretty cool :) Thank you so much!

@rusty1s rusty1s enabled auto-merge (squash) September 11, 2023 07:41
@rusty1s rusty1s disabled auto-merge September 11, 2023 07:42
@rusty1s rusty1s merged commit 3b97746 into pyg-team:master Sep 11, 2023
@ArchieGertsman ArchieGertsman deleted the lcm branch September 11, 2023 17:13
JakubPietrakIntel pushed a commit that referenced this pull request Sep 27, 2023
Initial version of the LCM aggregation, requested in issue #7574. A
possible feature to incorporate is for `forward` to additionally compute
and return an associativity loss, as described in the paper.

---------

Co-authored-by: Rishi Puri <puririshi98@berkeley.edu>
Co-authored-by: Jintang Li <cnljt@outlook.com>
Co-authored-by: rusty1s <matthias.fey@tu-dortmund.de>
@Kroppeb
Copy link

Kroppeb commented Dec 3, 2024

Why was the associativity loss not included?

@ArchieGertsman
Copy link
Contributor Author

Why was the associativity loss not included?

Hi @Kroppeb! I briefly attempted to implement associativity loss, but it wasn't as straightforward as commutativity loss and I never got around to finishing it. But good point - it should be available here.

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.

5 participants