-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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
GNNExplainer
improvements
#6065
Conversation
Codecov Report
@@ Coverage Diff @@
## master #6065 +/- ##
==========================================
+ Coverage 84.28% 84.30% +0.02%
==========================================
Files 362 362
Lines 20471 20444 -27
==========================================
- Hits 17254 17236 -18
+ Misses 3217 3208 -9
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
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.
Overall this implementation is very clean, thanks.
We might get incorrect results if the nodes in index
have overlapping subgraphs. Because some edges/nodes might get more importance as they are part of multiple subgraphs.
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.
LGTM! :) As Jinu suggested, I also think it's cleaner if the configs (explainer and model) are e.g. class variables and not passed through multiple methods, but we can do that in a future PR.
subgraph_edge_mask = subgraph[3][non_loop_mask] | ||
targets.append(data.edge_label[subgraph_edge_mask].cpu()) | ||
preds.append(expl_edge_mask[subgraph_edge_mask].cpu()) | ||
_, _, _, hard_edge_mask = k_hop_subgraph(node_index, num_hops=3, |
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.
I would assume that the edges between the 3-hop nodes and the 4-hop nodes have some importance, due to the GCN normalization, right?
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.
4-hop nodes do not have any importance as the layer is only 3 layer deep.
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.
But isn't the degree calculated with the information on the 4 hop nodes? And, therefore, the output would change if we would remove all the 4 hop nodes?
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.
yes, but the 4 hop nodes do not have any importance score as these edge masks are never trained.
subgraph
computation.GNNExplainer
is now always applied on the full set of edges. This fixes several bugs, e.g., thatindex
now references invalid entries. Instead, we compute the hard mask to get all edges involved in message passing.GNNExplainer
now correctly works with a tensor ofindex
target_index
clean-up.model.eval()
to the base class and resets it state afterwards.