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

Enh: deprecate legacy plots and code #3209

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

arshiaar
Copy link
Contributor

@arshiaar arshiaar commented Aug 22, 2023

Overview

Part of #3156

Description of the changes proposed in this pull request:

  • Added deprecation decorators using sklearn deprecated since its an existing dependency.
  • Decorators added to: bar_legacy, summary_legacy, dependence_legacy, waterfall_legacy
  • Added unit tests for dependence_legacy, summary_legacy, and waterfall_legacy.
  • Added decorator for kmeans in utils/._legacy.py

Potentially could create own decorator using the python warnings module.
Did not include unit tests for bar_legacy as function is breaking outside of decorator.

Checklist

  • All pre-commit checks pass.
  • Unit tests added (if fixing a bug or adding a new feature)

@dsgibbons
Copy link
Collaborator

Maybe update the message to say something like "will be removed in 0.44"?

@arshiaar arshiaar changed the title Enh/deprecate legacy plots Enh:deprecate legacy plots Aug 22, 2023
@arshiaar arshiaar changed the title Enh:deprecate legacy plots Enh: deprecate legacy plots and code Aug 22, 2023
@arshiaar arshiaar marked this pull request as ready for review August 22, 2023 11:18
@codecov
Copy link

codecov bot commented Aug 22, 2023

Codecov Report

Patch coverage: 100.00% and project coverage change: +0.11% 🎉

Comparison is base (3bd5d66) 57.58% compared to head (2665dc4) 57.70%.
Report is 5 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #3209      +/-   ##
==========================================
+ Coverage   57.58%   57.70%   +0.11%     
==========================================
  Files          88       88              
  Lines       12511    12521      +10     
==========================================
+ Hits         7205     7225      +20     
+ Misses       5306     5296      -10     
Files Changed Coverage Δ
shap/plots/_bar.py 60.36% <100.00%> (+0.36%) ⬆️
shap/plots/_beeswarm.py 79.69% <100.00%> (+1.43%) ⬆️
shap/plots/_scatter.py 48.43% <100.00%> (+0.23%) ⬆️
shap/plots/_waterfall.py 86.11% <100.00%> (+0.08%) ⬆️
shap/utils/_legacy.py 87.57% <100.00%> (+0.14%) ⬆️

... and 2 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Collaborator

@thatlittleboy thatlittleboy left a comment

Choose a reason for hiding this comment

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

Few suggestions (up for discussion, except the last one):

  • Use the phrase "marked for deprecation" instead of "deprecated". I've always found it confusing myself, but maybe it's just me.
  • Maybe shap v0.43.0 instead of Version 0.43.0. I'm thinking to specify "shap" because there might be wrapper libraries around shap, and I'm afraid end users will confuse 0.43 for the wrapper library's version rather than shap's.
  • Mention what the alternative should be. "Please use shap.plots.bar instead" or words to that effect. For summary_legacy, maybe just nudge the user towards using the dedicated shap.plots functions.
  • The docstrings of the legacy functions should be updated as well with the deprecation message & reference to the suggested alternative, in a deprecation admonition / warning box.

@thatlittleboy thatlittleboy added awaiting feedback Indicates that further information is required from the issue creator BREAKING Indicates that a PR is introducing a breaking change labels Oct 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awaiting feedback Indicates that further information is required from the issue creator BREAKING Indicates that a PR is introducing a breaking change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants