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

Graph Relational Attention #4031

Merged
merged 50 commits into from
Feb 21, 2022
Merged

Graph Relational Attention #4031

merged 50 commits into from
Feb 21, 2022

Conversation

fork123aniket
Copy link
Contributor

Added Relational Graph Attention Networks, which computes attention coefficients for relational graphs for each relation type for any number of relations and attention heads. Moreover, also provides functionality for edge attributes to handle.

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.

This is great. Left a few comments.

Note that we also need to add RGATConv to nn/conv/__init__.py. Furthermore, it would be great if you can add some tests for RGATConv in /test/nn/conv/test_rgat_conv.py (similar to other tests in that folder). Let me know if you need any help for this!

examples/rgat_conv.py Outdated Show resolved Hide resolved
examples/rgat_conv.py Outdated Show resolved Hide resolved
examples/rgat_conv.py Outdated Show resolved Hide resolved
examples/rgat_conv.py Outdated Show resolved Hide resolved
examples/rgat_conv.py Outdated Show resolved Hide resolved
torch_geometric/nn/conv/rgat_conv.py Show resolved Hide resolved
torch_geometric/nn/conv/rgat_conv.py Show resolved Hide resolved
torch_geometric/nn/conv/rgat_conv.py Outdated Show resolved Hide resolved
torch_geometric/nn/conv/rgat_conv.py Outdated Show resolved Hide resolved
torch_geometric/nn/conv/rgat_conv.py Outdated Show resolved Hide resolved
@fork123aniket
Copy link
Contributor Author

@rusty1s All the issues raised have been addressed. Two issues above are still open with comments made by me for each of them. Please read them and let me know if any further modification somewhere is required. Furthermore, have also added RGATConv to nn/conv/__init__.py.

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.

Thank you for the updates. This looks great!

Looking at documentation (https://pytorch-geometric--4031.org.readthedocs.build/en/4031/modules/nn.html#torch_geometric.nn.conv.RGATConv), can we make the text of formulas non-italic? For example via \text{(additive attention logits)}.

Furthermore, once a few tests lands I think this is ready to merge. Let me know if you need any guidance in doing so.

@fork123aniket
Copy link
Contributor Author

fork123aniket commented Feb 13, 2022

Have made the text of formulas non-italic in the documentation (https://pytorch-geometric--4031.org.readthedocs.build/en/4031/modules/nn.html#torch_geometric.nn.conv.RGATConv). Furthermore, have also added few tests, as can be seen in test_rgat_conv.py (committed to this pull request just now). Have also replied to the Torch Cuda device line addition to the rgat_conv.py (example) file issue above, which you can see in one of the resolved issues mentioned above.

@rusty1s
Copy link
Member

rusty1s commented Feb 14, 2022

Thanks you for your updates and being so responsive! I will have some last review today or tomorrow and will try to quickly merge afterwards.

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.

Thanks for the updates. LGTM.

@rusty1s rusty1s merged commit fbd0314 into pyg-team:master Feb 21, 2022
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

2 participants