-
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
Graph Explainability #4507
Graph Explainability #4507
Conversation
for more information, see https://pre-commit.ci
Codecov Report
@@ 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
Continue to review full report at Codecov.
|
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. |
# Conflicts: # torch_geometric/nn/models/GraphMask.py
for more information, see https://pre-commit.ci
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?? |
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. |
+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 |
# Conflicts: # torch_geometric/nn/models/GraphMask.py
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
@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. |
@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!!!! |
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 |
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.
@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.
|
||
|
||
def explain_message(self, out, x_i, x_j): | ||
basis_messages = Sequential(LayerNorm(out.size(-1)), ReLU())(out) |
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.
It looks like we are initializing a new model for every forward iteration. Is this intended?
@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!!! |
@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). |
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 What do you think? Let me know if you want a hand. |
@Padarn Thanks for suggesting a few ideas that can help take up this PR forward. I think these 6 functions ( As advised in point 2, it makes sense to remove the 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. |
@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. |
Thank you @fork123aniket. We still have plans to merge this but it is currently not top-prio on our roadmap. Sorry for the delay. |
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. |
Hey @fork123aniket, thanks for the reminder. Did you have a look at the new |
Hi @rusty1s, based on your recommendation, I've made changes to the Thank you... |
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. |
Closing this PR as new PR (#6284) has just been created for this implementation. |
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.