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

Add GMNConv and an example on ZINC #6775

Open
wants to merge 32 commits into
base: master
Choose a base branch
from
Open

Conversation

asarigun
Copy link

Adding The Graph Mixer Networks (https://arxiv.org/abs/2301.12493) Conv operator and example on ZINC

@rusty1s rusty1s changed the title Add 'GMNConv' and an example on 'ZINC' Add GMNConv and an example on ZINC Feb 23, 2023
@codecov
Copy link

codecov bot commented Feb 23, 2023

Codecov Report

Merging #6775 (0af668c) into master (f71ead8) will increase coverage by 0.01%.
Report is 5 commits behind head on master.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #6775      +/-   ##
==========================================
+ Coverage   87.40%   87.42%   +0.01%     
==========================================
  Files         473      474       +1     
  Lines       28570    28631      +61     
==========================================
+ Hits        24972    25030      +58     
- Misses       3598     3601       +3     
Files Coverage Δ
torch_geometric/nn/conv/__init__.py 100.00% <100.00%> (ø)
torch_geometric/nn/conv/gmn_conv.py 100.00% <100.00%> (ø)

... and 8 files with indirect coverage changes

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

Copy link
Author

@asarigun asarigun left a comment

Choose a reason for hiding this comment

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

I hope you find the implementation fully functional for PyG!

Copy link
Author

@asarigun asarigun left a comment

Choose a reason for hiding this comment

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

Reviewed the changes for GMNConv

@dufourc1 dufourc1 self-requested a review April 25, 2023 17:07
Copy link
Member

@dufourc1 dufourc1 left a comment

Choose a reason for hiding this comment

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

Hi! Thanks for the implementation, I'll read the paper linked to make sure I'm aligned, you'll probably just need to add some tests.

Copy link
Member

@dufourc1 dufourc1 left a comment

Choose a reason for hiding this comment

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

Hi, sorry for the delay and thanks again for the implementation ! Here are a few notes on the code and some more general comments.

Parts of the method are similar to PNA (PNAConv). I wonder if there is a way of reusing the code already available ? What do you think ?

For the tests you might want to check what is done here https://github.com/pyg-team/pytorch_geometric/blob/master/test/nn/conv/test_pna_conv.py.

torch_geometric/nn/conv/gmn_conv.py Outdated Show resolved Hide resolved
torch_geometric/nn/conv/gmn_conv.py Outdated Show resolved Hide resolved
torch_geometric/nn/conv/gmn_conv.py Outdated Show resolved Hide resolved
torch_geometric/nn/conv/gmn_conv.py Outdated Show resolved Hide resolved
examples/gmn.py Outdated Show resolved Hide resolved
@asarigun
Copy link
Author

asarigun commented May 20, 2023

Hi! Thanks for the feedback! I rearranged the code that you mentioned in the comments.

Hi, sorry for the delay and thanks again for the implementation ! Here are a few notes on the code and some more general comments.

Parts of the method are similar to PNA (PNAConv). I wonder if there is a way of reusing the code already available ? What do you think ?

For the tests you might want to check what is done here https://github.com/pyg-team/pytorch_geometric/blob/master/test/nn/conv/test_pna_conv.py.

@dufourc1 dufourc1 self-requested a review May 21, 2023 12:28
Copy link
Member

@dufourc1 dufourc1 left a comment

Choose a reason for hiding this comment

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

Sorry for the delay, thanks a lot for the requested changes 👍🏽

I have added a few comments, but maybe it's only because I am confused with the similarity with PNAConv.

torch_geometric/nn/conv/gmn_conv.py Outdated Show resolved Hide resolved
torch_geometric/nn/conv/gmn_conv.py Outdated Show resolved Hide resolved
test/nn/conv/test_gmn_conv.py Outdated Show resolved Hide resolved
torch_geometric/nn/conv/gmn_conv.py Outdated Show resolved Hide resolved
@asarigun
Copy link
Author

asarigun commented Oct 9, 2023

Hello @dufourc1,

Thank you for your insights on the code. I have made updates in the sections you highlighted and added comments where you've provided feedback.

Sorry for the delay, thanks a lot for the requested changes 👍🏽

I have added a few comments, but maybe it's only because I am confused with the similarity with PNAConv.

Copy link
Member

@dufourc1 dufourc1 left a comment

Choose a reason for hiding this comment

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

Hi ! The main points look good to me. Once you've updated your code to pass the CI tests,
this should be all good.

@asarigun
Copy link
Author

asarigun commented Oct 9, 2023

Thank you so much!

Hi ! The main points look good to me. Once you've updated your code to pass the CI tests, this should be all good.

@asarigun asarigun reopened this Oct 9, 2023
@dufourc1 dufourc1 self-requested a review October 10, 2023 05:03
@asarigun
Copy link
Author

Hi @dufourc1,

The code has successfully passed the CI checks! Thank you once again for your feedback🙌.

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.

None yet

3 participants