-
Notifications
You must be signed in to change notification settings - Fork 3.8k
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
Conversation
Codecov Report
@@ 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
📣 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.
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
.
@@ -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 |
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.
Given this do we also plan to move ThresholdConfig
args to the top level?
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 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
.
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.
Looks good.
The
Explainer
should take theExplainerConfig
arguments as top-level arguments.