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

Graph Explainability #4507

Closed
wants to merge 19 commits into from
Closed

Graph Explainability #4507

wants to merge 19 commits into from

Conversation

fork123aniket
Copy link
Contributor

This PR is an extension of discussion #4440. Moreover, this PR contains implementation of how to compute layer-wise weights for each edge in order to produce explanations for both node-level and graph-level tasks. Furthermore, this implementation is different from authors' original implementation and is fast and more memory efficient than theirs. Will add tests for this implementation to this PR later. @rusty1s Please take a look into this and let me know of any change which needs to be made in order to make this implementation model-agnostic.

@codecov
Copy link

codecov bot commented Apr 20, 2022

Codecov Report

Merging #4507 (de4f8f7) into master (d268e86) will increase coverage by 0.53%.
The diff coverage is 97.98%.

@@            Coverage Diff             @@
##           master    #4507      +/-   ##
==========================================
+ Coverage   82.77%   83.30%   +0.53%     
==========================================
  Files         315      317       +2     
  Lines       16512    17072     +560     
==========================================
+ Hits        13667    14221     +554     
- Misses       2845     2851       +6     
Impacted Files Coverage Δ
torch_geometric/nn/models/graphmask_explainer.py 97.98% <97.98%> (ø)
torch_geometric/nn/models/__init__.py 100.00% <100.00%> (ø)
torch_geometric/nn/glob/glob.py 86.84% <0.00%> (-13.16%) ⬇️
torch_geometric/nn/to_hetero_transformer.py 95.20% <0.00%> (-1.06%) ⬇️
torch_geometric/data/in_memory_dataset.py 77.27% <0.00%> (-0.67%) ⬇️
torch_geometric/data/download.py 100.00% <0.00%> (ø)
torch_geometric/utils/__init__.py 100.00% <0.00%> (ø)
torch_geometric/nn/conv/tag_conv.py 100.00% <0.00%> (ø)
torch_geometric/nn/glob/__init__.py 100.00% <0.00%> (ø)
torch_geometric/transforms/__init__.py 100.00% <0.00%> (ø)
... and 6 more

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 d268e86...de4f8f7. Read the comment docs.

@rusty1s
Copy link
Member

rusty1s commented Apr 21, 2022

Hi @fork123aniket, any reason you deleted your previous PR in the first place? Can you still incorporate the review suggestions from @Padarn in #4462. On a first look, it would be great to have some basic tests added to see how the module is being used in practice.

fork123aniket and others added 3 commits April 21, 2022 13:08
@fork123aniket
Copy link
Contributor Author

Oh, unfortunately the repo accidently got deleted altogether yesterday. Nevertheless, took all the reviews from @Padarn into consideration and made careful changes to the implementation. Besides, what if I add a quick example of how this implementation works for both node-level as well as graph-level tasks?? Will this be useful?? WDYT??

@rusty1s
Copy link
Member

rusty1s commented Apr 21, 2022

That's definitely useful, but I would prefer it as a follow-up PR. Let's start with adding tests first. Let me know if you need any help/advice for that.

@Padarn
Copy link
Contributor

Padarn commented Apr 21, 2022

+1 to some tests - I did a quick review of the code and was confused by a few things, tests are usually a great way to also help other know what the code is meant to do

@fork123aniket
Copy link
Contributor Author

@rusty1s Have just added few tests as well as few examples of how this implementation works. Please review all the files and let me know if anything more is required.

@fork123aniket
Copy link
Contributor Author

@rusty1s Am still awaiting any review comments from you. It's been 10 days since I committed the new files here. Please consider dropping some comments here to expedite the merge process. Thanks!!!!

torch_geometric/nn/models/GraphMask.py Outdated Show resolved Hide resolved
torch_geometric/nn/models/GraphMask.py Outdated Show resolved Hide resolved
torch_geometric/nn/models/GraphMask.py Outdated Show resolved Hide resolved
torch_geometric/nn/models/GraphMask.py Outdated Show resolved Hide resolved
torch_geometric/nn/models/GraphMask.py Outdated Show resolved Hide resolved
@Padarn
Copy link
Contributor

Padarn commented May 20, 2022

I added some comments @fork123aniket - including some I made before and didn't seem to be addressed.

Note that CI is failing due to the changelog check (you're required to update CHANGELOG.md)

Copy link
Member

@rusty1s rusty1s left a comment

Choose a reason for hiding this comment

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

@fork123aniket Thanks for the PR. This is a pretty complex PR so it takes us some time to understand the paper and the implementation.

I think we first need to agree on the interface before going further. Left some comments and questions below.

examples/graphmask_explainer.py Show resolved Hide resolved
examples/graphmask_explainer.py Show resolved Hide resolved


def explain_message(self, out, x_i, x_j):
basis_messages = Sequential(LayerNorm(out.size(-1)), ReLU())(out)
Copy link
Member

Choose a reason for hiding this comment

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

It looks like we are initializing a new model for every forward iteration. Is this intended?

test/nn/models/test_graphmask_explainer.py Show resolved Hide resolved
examples/graphmask_explainer.py Show resolved Hide resolved
@fork123aniket
Copy link
Contributor Author

@rusty1s It's been 17 days since I received last review comments. Do you have any other comments or could you please suggest the next steps to merge this PR?? Thanks!!!

@rusty1s
Copy link
Member

rusty1s commented Jun 9, 2022

@fork123aniket Much going on at the moment, sorry for the delay. I can start another round of review soon, but I see that a lot of comments are not resolved yet, e.g., #4507 (comment).

@Padarn
Copy link
Contributor

Padarn commented Jul 10, 2022

Hey @fork123aniket I think this is a nice addition but I also feel it might be hard for us to effectively review and align on such a big PR.

I'm wondering if we should try and split it into a few smaller PRs? A few possible ideas from the top of my head would be to 1) split out the core interfaces you are proposing to add to get hooks into the MessagePassing 2) remove the visualization code and put into a new PR.

What do you think? Let me know if you want a hand.

@fork123aniket
Copy link
Contributor Author

@Padarn Thanks for suggesting a few ideas that can help take up this PR forward.

I think these 6 functions (enable_layer, reset_parameters, freeze_model, set_flags, inject_messages, explain_message) can be included directly in the Explainer BaseClass. I know some of these may seem Explainer-specific, however I see this move as the only way to make the overall code (graphmask_explainer.py) somewhat concise. Moreover, other proposed functions can be considered to remain in the GraphMaskExplainer Class as it makes the interface of both GNNExplainer and GraphMaskExplainer classes more or less similar.

As advised in point 2, it makes sense to remove the visualize_subgraph() altogether from this PR for now and chase it with the new one at a later point in time. Besides, one thing that we can't let go unnoticed is that all the remaining three files (examples/graphmask_explainer.py, test_graphmask_explainer.py, and __init__.py) must be excluded from this PR as well at least until we get over with the final interface design discussion for this GraphMaskExplainer Class. What do you guys think @Padarn @rusty1s ??

Nevertheless, I am open to discussing more ideas to expedite the merge process. Please let me know if the approach written in para 2 seems more promising or any other thoughts that can help get started with the modification.

@fork123aniket
Copy link
Contributor Author

@Padarn @rusty1s despite commenting on the suggestions mentioned above, I don't see any more comments from you about taking this PR forward. Please let me know if we can close this PR or else just put some comments here about splitting this big PR into smaller pieces so that we can include this model-agnostic graph explainability technique in PyG.

Thank you. Looking forward to hearing from you very soon.

@rusty1s
Copy link
Member

rusty1s commented Sep 2, 2022

Thank you @fork123aniket. We still have plans to merge this but it is currently not top-prio on our roadmap. Sorry for the delay.

@fork123aniket
Copy link
Contributor Author

Hi @rusty1s @Padarn,

I received the first-ever PyG newsletter sometime earlier last month and I noticed that there is a sprint already kicked off to include Graph Explainability capabilities in PyG. Moreover, I read through the roadmap (#5520) on the same. To the best of my knowledge, the implementation associated with this PR has the ability to run on both homogeneous as well as heterogeneous graphs and I believe completing this PR will add more value to PyG as we're trying to make the implementation more model-agnostic.

It's been more than half a year time since I last received any comments from you guys on this. I literally wanna get this PR merged so that PyG has some cool functionalities that this PR could bring. Hope, you both understand my intentions. So, is it now still possible to take this PR forward??

Thank you. Looking forward to hearing from you very soon.

@rusty1s
Copy link
Member

rusty1s commented Dec 7, 2022

Hey @fork123aniket, thanks for the reminder. Did you have a look at the new torch_geometric.explain package? What we could do is try to move this PR over to be based on the new ExplainerAlgorithm base class.

@fork123aniket
Copy link
Contributor Author

Hi @rusty1s,

based on your recommendation, I've made changes to the GraphMask Explainability approach so that it works on the basis of the new ExplainerAlgorithm base class and also called the approach from inside the new Explainer class (the same as GNNExplainer calls from inside the new Explainer class in torch_geometric.explain package). Based on the examples implemented thus far, it works perfectly for layer_types such as RGCNConv, GCNConv, and GATConv for Node Classification, Graph Classification, and Link Prediction tasks. Since it works for RGCNConv, hence we can say the approach can work for any heterogeneous graphs along with its support for homogeneous graphs. Please let me know if new PR can be created on this with all the new files (approach, approach_example, and approach_tests) committed to the same??

Thank you...

@rusty1s
Copy link
Member

rusty1s commented Dec 20, 2022

This is cool to hear. Do you want to add this in a new PR? If so, let's try to split it into two (1 for functionality and tests, 1 for the missing example) to ease reviewing, and I will ensure that you will get reviews in time.

@fork123aniket fork123aniket closed this by deleting the head repository Dec 22, 2022
@fork123aniket
Copy link
Contributor Author

Closing this PR as new PR (#6284) has just been created for this implementation.

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

Successfully merging this pull request may close these issues.

3 participants