-
Notifications
You must be signed in to change notification settings - Fork 47
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
extended GatedGraphConv and NNConv to use AbstactGNNGraph #375
base: master
Are you sure you want to change the base?
Conversation
Hi @CarloLucibello. Is this correct or did I misunderstand something? |
Thanks for this contribution! It would be nice to have some tests |
Should I modify the already made tests for both NNConv and GatedGraphConv to use AbstactGNNGraph. or should I implement new tests. And I was planning on resolving the whole issue should I continue in the same manner? |
@@ -589,7 +589,7 @@ end | |||
# remove after https://github.com/JuliaDiff/ChainRules.jl/pull/521 | |||
@non_differentiable fill!(x...) | |||
|
|||
function (l::GatedGraphConv)(g::GNNGraph, H::AbstractMatrix{S}) where {S <: Real} | |||
function (l::GatedGraphConv)(g::AbstractGNNGraph, H::AbstractMatrix{S}) where {S <: Real} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just this change is not enough to make this layer compatible with heterograph
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, could you tell me what I should add?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
and is NNConv ok or should I also modify it?
you can add tests here |
This is an attempt in solving #311 partially by extending both NNConv and GatedGraphConv.