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

Move ExplainerConfig arguments to Explainer class #6176

Merged
merged 5 commits into from
Dec 8, 2022

Conversation

rusty1s
Copy link
Member

@rusty1s rusty1s commented Dec 7, 2022

The Explainer should take the ExplainerConfig arguments as top-level arguments.

@codecov
Copy link

codecov bot commented Dec 7, 2022

Codecov Report

Merging #6176 (e30dbf5) into master (3413d9a) will decrease coverage by 0.00%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #6176      +/-   ##
==========================================
- Coverage   84.37%   84.37%   -0.01%     
==========================================
  Files         366      366              
  Lines       20544    20548       +4     
==========================================
+ Hits        17335    17338       +3     
- Misses       3209     3210       +1     
Impacted Files Coverage Δ
torch_geometric/explain/config.py 100.00% <ø> (ø)
torch_geometric/explain/explainer.py 100.00% <100.00%> (ø)
torch_geometric/sampler/neighbor_sampler.py 66.39% <0.00%> (-0.14%) ⬇️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Copy link
Member

@wsad1 wsad1 left a comment

Choose a reason for hiding this comment

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

Did you feel the previous way of passing explainer_config was less readable?. Now the docs feel a little weird becuase to understand node_mask_type better one has to look at a different class ExplainerConfig.

torch_geometric/explain/explainer.py Outdated Show resolved Hide resolved
@@ -23,27 +24,49 @@ class Explainer:
Args:
model (torch.nn.Module): The model to explain.
algorithm (ExplainerAlgorithm): The explanation algorithm.
explainer_config (ExplainerConfig): The explainer configuration.
explanation_type (ExplanationType or str): The type of explanation to
Copy link
Member

Choose a reason for hiding this comment

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

Given this do we also plan to move ThresholdConfig args to the top level?

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't plan to. Discussed with @RBendias and we both found it weird that the Explainer expects an ExplainerConfig - these should be top-level args. I still would like to keep the grouping of args of ModelConfig and ThresholdConfig.

torch_geometric/explain/explainer.py Outdated Show resolved Hide resolved
Copy link
Member

@wsad1 wsad1 left a comment

Choose a reason for hiding this comment

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

Looks good.

@rusty1s rusty1s merged commit c42940b into master Dec 8, 2022
@rusty1s rusty1s deleted the drop_explainer_config branch December 8, 2022 09:43
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.

None yet

2 participants