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

API: Multi-output summary plot legend #3062

Merged
merged 12 commits into from Jul 16, 2023

Conversation

101AlexMartin
Copy link
Contributor

@101AlexMartin 101AlexMartin commented Jul 5, 2023

Overview

Added flag to allow the printing of the mean SHAP values in the legend of a multi-output summary bar plot. The motivation behind this change is to easily compare rankings when different chunks of data are used to get the SHAP values. Comparing visually the overall ranking in a multi-output problem was quite challenging, so this pull request solves the problem.

Checklist

  • Closes #xxxx
  • All pre-commit checks pass.
  • Unit tests added (if fixing a bug or adding a new feature)
  • Added entry to CHANGELOG.md (if changes will affect users)

@thatlittleboy
Copy link
Collaborator

Hi @101AlexMartin , thanks for the PR!

A couple of things before we kick off the PR review:

  • Please fill up the checklist in the PR description.
  • As this is a new feature, please add tests to ensure the desired functionality is implemented properly.
  • I intend to deprecate summary_legacy at some point in the future. So I'll also suggest adding this param into _bar.py as well.

@thatlittleboy thatlittleboy added the awaiting feedback Indicates that further information is required from the issue creator label Jul 6, 2023
@101AlexMartin
Copy link
Contributor Author

Hello! I implemented your proposed changes :)
As for the depreciation of summary_legacy and the addition of the parameter to _bar.py as well. If I'm not mistaken, _bar.py is not ready yet to display graphs for multiouput models. Thus, I do not see the addition of this parameter feasible.

@thatlittleboy
Copy link
Collaborator

Hi @101AlexMartin , next steps would be to make sure the tests pass :)

In particular for plotting tests, we are using pytest-matplotlib. You will need to generate the baseline images and commit that into the repository.

This way we can ensure that the function always generates the same-looking plot.

@101AlexMartin
Copy link
Contributor Author

101AlexMartin commented Jul 8, 2023

I added the tests and the baseline figures :)
Also merged with master branch

@codecov
Copy link

codecov bot commented Jul 9, 2023

Codecov Report

Merging #3062 (73ff252) into master (d650456) will increase coverage by 0.02%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #3062      +/-   ##
==========================================
+ Coverage   55.33%   55.35%   +0.02%     
==========================================
  Files          90       90              
  Lines       12875    12883       +8     
==========================================
+ Hits         7124     7132       +8     
  Misses       5751     5751              
Impacted Files Coverage Δ
shap/plots/_beeswarm.py 78.25% <100.00%> (+0.34%) ⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@thatlittleboy thatlittleboy removed the awaiting feedback Indicates that further information is required from the issue creator label Jul 9, 2023
@101AlexMartin
Copy link
Contributor Author

What is the status of this then?
Let me know if you need any extra explanation :)

@thatlittleboy
Copy link
Collaborator

@101AlexMartin Sorry, let me get back to you over the weekend. Bit busy at work this week.

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.

Hi @101AlexMartin Sorry this review took so long! This should be the last round, I just have a couple more suggestions for you to make.

shap/plots/_beeswarm.py Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
shap/plots/_beeswarm.py Outdated Show resolved Hide resolved
@thatlittleboy thatlittleboy added the enhancement Indicates new feature requests label Jul 15, 2023
@101AlexMartin
Copy link
Contributor Author

Done :)

@thatlittleboy
Copy link
Collaborator

thatlittleboy commented Jul 15, 2023

Just a heads up, the 3.11 tests are failing, not anything to do with your PR. Probably due to the 4.0 lightgbm release. #3092

This PR looks good to me, I'll come back to this once we get our builds running again. Hang tight!

@thatlittleboy thatlittleboy added this to the 0.43.0 milestone Jul 16, 2023
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.

LGTM

dog-teaches-baby-crawling

@thatlittleboy thatlittleboy merged commit 123ceaa into shap:master Jul 16, 2023
15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Indicates new feature requests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants