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

Adds GATv2 layer #97

Merged
merged 9 commits into from
Jan 7, 2022
Merged

Adds GATv2 layer #97

merged 9 commits into from
Jan 7, 2022

Conversation

abieler
Copy link
Contributor

@abieler abieler commented Jan 5, 2022

Adds GATv2 as from https://arxiv.org/abs/2105.14491.

Did the same thing over at GeometricFlux.jl Pull Request. Just as I was "done" I stumbled upon this repo and liked the codebase a bit better.

The variable names in GATv2 deviates a bit from the GAT layer, i.e. weight -> W, and aWW -> eij to stick a bit closer to the notation in the paper. If you want to keep it consistent across the code base I can revert of course.

Cheers
Andre

@CarloLucibello
Copy link
Member

thanks, very neat PR, and fixes #74. It's ok to have a naming convention matching the paper.

@abieler
Copy link
Contributor Author

abieler commented Jan 5, 2022

Cool. Did not see that there was an open issue for that!

src/layers/conv.jl Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Jan 5, 2022

Codecov Report

Merging #97 (9e9a9ed) into master (c13af48) will increase coverage by 0.05%.
The diff coverage is 86.84%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #97      +/-   ##
==========================================
+ Coverage   83.85%   83.91%   +0.05%     
==========================================
  Files          14       14              
  Lines        1109     1144      +35     
==========================================
+ Hits          930      960      +30     
- Misses        179      184       +5     
Impacted Files Coverage Δ
src/layers/conv.jl 77.59% <86.84%> (+1.04%) ⬆️

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 c13af48...9e9a9ed. Read the comment docs.

src/layers/conv.jl Outdated Show resolved Hide resolved
Co-authored-by: Carlo Lucibello <[email protected]>
src/layers/conv.jl Outdated Show resolved Hide resolved
@CarloLucibello
Copy link
Member

I find the use of W in eqs. 4 and 7 in the paper a bit inconsistent, but hopefully your implementation should reflect the intent of the authors. Also, there is a footnote on page 5:
"We also add a bias vector b before applying the nonlinearity, we omit this in Equation (7) for brevity"
Should we add it?

@abieler
Copy link
Contributor Author

abieler commented Jan 5, 2022

Now that you say that... I actually had the additional bias in the code, but could not remember why I put it there by looking through the formulas, so I removed it before submitting the PR xD.
I ll check Pytorch Geom and DGL tomorrow to see what they did

@CarloLucibello
Copy link
Member

"We also add a bias vector b before applying the nonlinearity, we omit this in Equation (7) for brevity"
Should we add it?

The easiest way would be to replace Wi and Wj in the constructor with

dense_i = Dense(..., bias=true)
dense_j = Dense(..., bias=false)

@abieler
Copy link
Contributor Author

abieler commented Jan 5, 2022

Yup. Thats also whats in DGL and PytorchGeometric. I ll make the change tomorrow. Thanks for the fast review

src/layers/conv.jl Outdated Show resolved Hide resolved
@CarloLucibello
Copy link
Member

Do you understand what is causing the error?

@abieler
Copy link
Contributor Author

abieler commented Jan 6, 2022

sorry, busy day, have not gotten around looking into it. I'll hollla should i get stuck.

src/layers/conv.jl Outdated Show resolved Hide resolved
src/layers/conv.jl Outdated Show resolved Hide resolved
test/layers/conv.jl Outdated Show resolved Hide resolved
@CarloLucibello
Copy link
Member

Needed just some small tweaks. Merging this, thanks again!

@CarloLucibello CarloLucibello merged commit 957ce36 into JuliaGraphs:master Jan 7, 2022
@abieler
Copy link
Contributor Author

abieler commented Jan 7, 2022

nice! thanks for maintaining this package, i think I'll test ride it a bit further on some use cases i have :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants