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

Added EdgeWeightNorm layer #158

Draft
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

rbSparky
Copy link
Contributor

@rbSparky rbSparky commented Apr 15, 2022

Verified outputs with the pytorch implementation, will add tests now.

@codecov
Copy link

codecov bot commented Apr 15, 2022

Codecov Report

Merging #158 (7ca7589) into master (104b6b9) will decrease coverage by 0.38%.
The diff coverage is 0.00%.

❗ Current head 7ca7589 differs from pull request most recent head 549b7ac. Consider uploading reports for the commit 549b7ac to get more accurate results

@@            Coverage Diff             @@
##           master     #158      +/-   ##
==========================================
- Coverage   85.52%   85.14%   -0.39%     
==========================================
  Files          15       14       -1     
  Lines        1285     1272      -13     
==========================================
- Hits         1099     1083      -16     
- Misses        186      189       +3     
Impacted Files Coverage Δ
src/layers/conv.jl 75.90% <0.00%> (-2.80%) ⬇️
src/GNNGraphs/gnngraph.jl 76.00% <0.00%> (-4.00%) ⬇️
src/GNNGraphs/transform.jl 96.27% <0.00%> (-0.44%) ⬇️
src/GNNGraphs/sampling.jl 100.00% <0.00%> (ø)
src/deprecations.jl
src/GNNGraphs/query.jl 93.02% <0.00%> (+0.04%) ⬆️
src/GNNGraphs/convert.jl 90.40% <0.00%> (+0.31%) ⬆️
src/GNNGraphs/utils.jl 84.72% <0.00%> (+10.23%) ⬆️

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 2801b51...549b7ac. Read the comment docs.

src/layers/conv.jl Outdated Show resolved Hide resolved
src/layers/conv.jl Outdated Show resolved Hide resolved

for iter in 1:length(edge_weight)
if l.norm_both
push!(norm_val, edge_weight[iter] / (sqrt(dg_out[in[iter]] * dg_in[out[iter]]) + l.eps))
Copy link
Owner

Choose a reason for hiding this comment

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

these mutating operations are not AD friendly. I didn't think about it carefully but you should probably use apply_edges here


for iter in 1:length(edge_weight)
if l.norm_both
push!(norm_val, edge_weight[iter] / (sqrt(dg_out[in[iter]] * dg_in[out[iter]]) + l.eps))
Copy link
Owner

Choose a reason for hiding this comment

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

these mutating operations are not AD friendly. I didn't think about it carefully but you should probably use apply_edges here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried doing this using aggregate_neighbours, but ran into the same issue of it not being AD friendly, main issue being that I'm currently computing ∑e_jk and ∑e_ki individually for each e_ji, which I guess is to be done with some sort of matrix multiplication.

I'm not sure yet how its to be done using apply_edges, but I'll look more into it(will try to understand how its done in pyTorch/DGL) and let you know any updates.

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