MAINT: Added deprecation warning to show argument for waterfall plot #4056
Conversation
|
Also, should I update the docs to reflect this change? Or should that wait until all the plot functions have been changed? |
shap/plots/_waterfall.py
Outdated
| if show is not None: | ||
| warnings.warn( | ||
| "WARNING: `show` argument is deprecated and will be removed in a future version. Please omit this argument and call plt.show() manually.", | ||
| category=UserWarning, |
There was a problem hiding this comment.
Should this be DeprecationWarning ?
There was a problem hiding this comment.
Thanks for the catch, fixed
shap/plots/_waterfall.py
Outdated
| # Deprecation warning | ||
| if show is not None: | ||
| warnings.warn( | ||
| "WARNING: `show` argument is deprecated and will be removed in a future version. Please omit this argument and call plt.show() manually.", |
There was a problem hiding this comment.
The WARNING: prefix is not necessary
| @@ -13,7 +15,7 @@ | ||
| # plot that is associated with that feature get overlaid on the plot...it would quickly allow users to answer | ||
| # why a feature is pushing down or up. Perhaps the best way to do this would be with an ICE plot hanging off | ||
| # of the bar... | ||
| def waterfall(shap_values, max_display=10, show=True): | ||
| def waterfall(shap_values, max_display=10, show=None): |
There was a problem hiding this comment.
Please let's not change the default here since this is a breaking change.
There was a problem hiding this comment.
Gotcha, I'll change that.
In that case, when should the deprecation warning trigger? Is there a way to detect whether show=True because it is the default argument versus it being explicitly passed by the user? If not, should the warning trigger whenever the function is called, regardless of the arguments used?
There was a problem hiding this comment.
Hmm, I am a bit reluctant to add this, since we use the show parameter in a lot of different plots and I would like to deprecate all of them at once.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #4056 +/- ##
=======================================
Coverage 65.63% 65.63%
=======================================
Files 94 94
Lines 13141 13144 +3
=======================================
+ Hits 8625 8627 +2
- Misses 4516 4517 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Overview
Implements #3819 (to be tracked on #3507)
Description of the changes proposed in this pull request:
I added a deprecation warning to the "show" argument in
_waterfall.py. If this looks good, I am happy to add a similar warning to all the other plotting functions as outlined in #3819.Checklist