-
Notifications
You must be signed in to change notification settings - Fork 87
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
Fix SHAP prediction explanations bug #3221
Conversation
Codecov Report
@@ Coverage Diff @@
## main #3221 +/- ##
=======================================
+ Coverage 99.8% 99.8% +0.1%
=======================================
Files 326 326
Lines 31388 31395 +7
=======================================
+ Hits 31297 31304 +7
Misses 91 91
Continue to review full report at Codecov.
|
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 to me @eccabay ! Thank you for the fix. Can you please run the model understanding perf tests to be sure we're not accidentally introducing a regression?
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.
LGTM, thank you @eccabay! Agreed with Freddy's suggestion about running perf tests, and I'm almost tempted to say that perhaps the identity method could be easier to understand than the logit link function :)
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.
Agreed with both above! Nice fix!
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.
Thank you!
@freddyaboulton great suggestion for perf testing! Results look pretty consistent to me, lmk what you think! |
@eccabay Thanks for running the tests! Send it! |
Closes #3220
Proposed solution is to remove the usage of the logit link function in our explanations altogether. The only difference between the function is what space the final SHAP values are reported in. With the logit function they sit in log-odds space, but between -1 and 1 with the identity function. Due to the lack of significant difference between the two methods, I figure there’s no significant impact in making this change.