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

Add support for passing check_additivity argument #103

Merged
merged 2 commits into from
Mar 16, 2021
Merged

Conversation

Matgrb
Copy link
Contributor

@Matgrb Matgrb commented Mar 16, 2021

Closes: #102

@Matgrb Matgrb requested a review from timvink March 16, 2021 09:29
Copy link
Contributor

@timvink timvink left a comment

Choose a reason for hiding this comment

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

Nice work Mateusz!

The PR will add support for the check_additivity argument, but users will keep running into the issue, have to lookup the docs, and change that settings.

Tools add more value if they can offload some cognitive overhead. Can we explicitly add approximate and check_additivity parameters (next to the **shap_kwargs) ? And then set a reasonable default? Which is probably check_additivity=True and explain in docs that to speed up you can set it to false.

@Matgrb
Copy link
Contributor Author

Matgrb commented Mar 16, 2021

The main difficulty here is that all shap_kwargs need to be passed to the __init__ method of Explainer, while these two are passed only to shap_values of the TreeExplainer. Explainer is transformed into TreeExplainer at initialization.
I agree that putting everything into **shap_kwargs would be easier, but also it would be harder to ensure correctness, because we would need to make sure that any parameter passed is put either to init or shap_values for all explainers.
What do you think? I will merge this one, and let's continue discussion how we can improve it.

@Matgrb Matgrb merged commit ec57ef5 into main Mar 16, 2021
@ReinierKoops ReinierKoops deleted the disable_check branch December 5, 2023 21:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Additivity check failed in TreeExplainer!
2 participants