FluxML / GeometricFlux.jl

Geometric Deep Learning for Flux
https://fluxml.ai/GeometricFlux.jl/stable/
MIT License
348 stars 30 forks source link

the GCNConv implementation is not correct #249

Closed CarloLucibello closed 2 years ago

CarloLucibello commented 2 years ago

The GCNConv implementation relies on the multiplication of the node features by the normalized Laplacian L̃ https://github.com/FluxML/GeometricFlux.jl/blob/c8396e4a34637c06c5c479851877c44e1a85a530/src/layers/conv.jl#L40 This is not what the original paper does, and one should use the normalized adjacency à instead. The two are related by the equation à = I - L̃

yuehhua commented 2 years ago

The GCNConv here is implemented based on equation 8 in the original paper, which is

D̃^-0.5 Ã D̃^-0.5 X Θ

where à = I + A. You may want to check the paper somewhere above equation 8.

Since D^-0.5 A D^-0.5 is so called normalized Laplacian, the adjacency matrix here is corrected by adding self loops, which is à = I + A. So normalized Laplacian with self-loop is

L̃ = D̃^-0.5 Ã D̃^-0.5

The two are related by the equation à = I - L̃

Thus, which is not correct.

CarloLucibello commented 2 years ago

Let me call B the normalized adjacency then: B = D^-0.5 Ã D^-0.5 The Laplacian is given by L = D - A while the normalized Laplacian is L̃ = I - B as you can see from your own implementation here https://github.com/yuehhua/GraphSignals.jl/blob/31110340851d82aa960e51a41548afdf5b3444c0/src/linalg.jl#L131 The implementation of the normalized laplacian is correct, it is consistent with what people normally call the normalized laplacian. The problem is that GCNConv is not supposed to use it, GCNConv's implementation is wrong.

yuehhua commented 2 years ago

The problem is that GCNConv is not supposed to use it

Yes, GCNConv doesn't use normalized Laplacian but use normalized Laplacian with self-loop, which is corrected by using à = I + A, instead of A directly. If you use A for computing normalized Laplacian, then that will be the normal one. But if you use for normalized Laplacian, then it will be normalized Laplacian with self-loop.

CarloLucibello commented 2 years ago

I'll try one more time.

What the GCNConv is supposed to do is

à = I + A
D = Diagonal(vec(sum(Ã, dims=1)))
B = D^{-0.5} * Ã * D^{-0.5}
y = B * x

What it does instead is

à = I + A
D = Diagonal(vec(sum(Ã, dims=1)))
L = I - D^{-0.5} * Ã * D^{-0.5}
y = L * x

Can you see the problem?

yuehhua commented 2 years ago

Oh! I see it now. Thank you so much.