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 waterfall color config for waterfall plot #3377

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

Conversation

CloseChoice
Copy link
Collaborator

Overview

Closes #3266

Description of the changes proposed in this pull request:

Checklist

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

Copy link

codecov bot commented Nov 2, 2023

Codecov Report

Attention: 1 lines in your changes are missing coverage. Please review.

Comparison is base (b99ab20) 58.18% compared to head (6c7093b) 58.25%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #3377      +/-   ##
==========================================
+ Coverage   58.18%   58.25%   +0.07%     
==========================================
  Files          89       89              
  Lines       12564    12588      +24     
==========================================
+ Hits         7310     7333      +23     
- Misses       5254     5255       +1     
Files Coverage Δ
shap/plots/__init__.py 90.00% <100.00%> (ø)
shap/plots/_waterfall.py 86.76% <96.77%> (+0.68%) ⬆️

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

@CloseChoice CloseChoice marked this pull request as ready for review November 2, 2023 22:53
@CloseChoice
Copy link
Collaborator Author

@thatlittleboy @connortann Some time passed since I opened this PR. Do you have time to look at this?

@connortann
Copy link
Collaborator

I've been away recently but back around now, so I'll have some time this week to have a proper look. From a brief read this looks great, but I'd like to give a proper review and think about design choices. This will be a great feature to include.

@CloseChoice
Copy link
Collaborator Author

I've been away recently but back around now, so I'll have some time this week to have a proper look. From a brief read this looks great, but I'd like to give a proper review and think about design choices. This will be a great feature to include.

Please note that IF we add this, we should probably use this pattern in the other plotting functions aswell. So that we have one dataclass for each plotting function and then we could generalize from there since all configuration options will be visible in the dataclasses.

@connortann
Copy link
Collaborator

connortann commented Nov 14, 2023

I think we should discuss the design choices and make a rough plan; I'd like to run some ideas past you. I'm not sure that a dataclass is the best way to proceed. I'll comment on the issue tracker over at #3266 ...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
visualization Relating to plotting
Projects
None yet
Development

Successfully merging this pull request may close these issues.

No native way to change colors for plots such as Waterfall plots
2 participants