-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
Heterogeneous Explanation #6091
Conversation
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.
Thanks, overall the interfaces look good. Left some initial comments.
Codecov Report
@@ Coverage Diff @@
## master #6091 +/- ##
==========================================
+ Coverage 84.49% 84.55% +0.05%
==========================================
Files 371 371
Lines 20741 20821 +80
==========================================
+ Hits 17525 17605 +80
Misses 3216 3216
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
I added logic and testing for thresholding, I am unsure if my implementation is good, overall having trouble extracting the Currently I am doing it like this x_dict = None
edge_index_dict = None
edge_attr_dict = None
for key, value in self.node_items():
if key == "x_dict":
x_dict = value
elif key == "edge_index_dict":
edge_index_dict = value
elif key == "edge_attr_dict":
edge_attr_dict = value
continue And setting the updated dictionaries like this: out.edge_index_dict.update(edge_index_dict)
out.edge_attr_dict.update(edge_attr_dict) @wsad1 Is there a better way to access the |
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.
Thanks for the updates @avgupta456 . Will take a look again once you are done with TODOs in test.
I'm currently running into an issue with HeteroExplanation that is blocking my further progress. Unlike Explanation (inherits from Data), HeteroExplanation (inherits from HeteroData) does not correctly store the x_dict, edge_index_dict, and edge_attrs_dict when initialized with these arguments. It does work if the dictionaries are set as attributes afterwards. HeteroExplanation(x_dict=x_dict, edge_index_dict=edge_index_dict, edge_attrs_dict=edge_attrs_dict) # does not work
explanation = HeteroExplanation() # does work
explanation.x_dict = x_dict
explanation.edge_index_dict = edge_index_dict
explanation.edge_attrs_dict = edge_attrs_dict One workaround is that when initialized with the dictionaries, they are accessible through self.node_items(). Perhaps this is a bug with HeteroData? Or maybe I am doing something else incorrectly. As a result of this bug, the subgraph method isn't working as intended. Open to any advice. |
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.
This looks clean now. Will approve once the subgraph
issue is resolved.
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.
Thanks for the work ! LGTM, these are nitpick comments
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.
Thanks @avgupta456, this looks good.
Starts heterogeneous explanations using the new
Explainer
framework. CreatesHeterogeneousExplanation
and extendsExplainer
,ExplainerAlgorithm
, andDummyExplainer
to handle heterogeneous graphs. Tests and functionalities are partially complete, looking to get some initial validation before continuing.Todo
Closes #6014