Skip to content

Fix shap.summary_plot to work with matplotlib 3.6.0 #2697

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 5 commits into from
Jul 12, 2023
Merged

Fix shap.summary_plot to work with matplotlib 3.6.0 #2697

merged 5 commits into from
Jul 12, 2023

Conversation

jklaise
Copy link
Contributor

@jklaise jklaise commented Sep 21, 2022

Fixes #2687. Have tested this manually by calling shap.summary_plot(...) to produce expected behaviour.

Note that there may be additional places where this is needed, e.g. for other plots, here's a grep of all the occurences of pl.colorbar in the codebase:

plots/_decision.py
151:        # place the colorbar
154:        cb = pl.colorbar(m, ticks=[0, 1], orientation="horizontal", cax=ax_cb)

plots/_monitoring.py
78:    cb = pl.colorbar()

plots/_embedding.py
68:    cb = pl.colorbar()

plots/_image.py
169:    cb = fig.colorbar(im, ax=np.ravel(axes).tolist(), label="SHAP value", orientation="horizontal",

plots/_heatmap.py
118:    #pl.colorbar()
137:        cb = pl.colorbar(m, ticks=[min(vmin,-vmax), max(-vmin,vmax)], aspect=1000, fraction=0.0090, pad=0.10,

plots/_violin.py
460:        cb = pl.colorbar(m, ticks=[0, 1], aspect=1000)

plots/_beeswarm.py
365:        cb = pl.colorbar(m, ax=pl.gca(), ticks=[0, 1], aspect=80)
865:        cb = pl.colorbar(m, ax=pl.gca(), ticks=[0, 1], aspect=80)

plots/_scatter.py
346:            cb = pl.colorbar(p, ticks=tick_positions, ax=ax, aspect=80)
349:            cb = pl.colorbar(p, ax=ax, aspect=80)
700:            cb = pl.colorbar(p, ticks=tick_positions, ax=ax, aspect=80)
703:            cb = pl.colorbar(p, ax=ax, aspect=80)

@jklaise jklaise mentioned this pull request Sep 21, 2022
Closed
@squangelo squangelo mentioned this pull request Oct 17, 2022
Closed
@dsgibbons dsgibbons mentioned this pull request Nov 26, 2022
Closed
55 tasks
@dsgibbons dsgibbons mentioned this pull request May 16, 2023
Merged
@codecov
Copy link

codecov bot commented Jul 11, 2023 •
edited
Loading

Codecov Report

Merging #2697 (9fbea17) into master (bbb8d2b) will not change coverage.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master    #2697   +/-   ##
=======================================
  Coverage   55.35%   55.35%           
=======================================
  Files          90       90           
  Lines       12873    12873           
=======================================
  Hits         7126     7126           
  Misses       5747     5747
Impacted Files Coverage Δ
shap/plots/_beeswarm.py 77.90% <100.00%> (ø)
shap/plots/_violin.py 81.12% <100.00%> (ø)

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

@connortann
Copy link
Collaborator

Thank you for the PR!

May I suggest also adding the same fix to a few other places in shap/plots:

  • _violin.py:L310
  • _monitoring.py:L73
  • _embedding.py:L63

I think the other uses of colorbar are ok, as they already provide the ax or cax argument.

@connortann connortann added the bug Indicates an unexpected problem or unintended behaviour label Jul 11, 2023
@connortann connortann added this to the 0.43.0 milestone Jul 11, 2023
@connortann
Copy link
Collaborator

PS - it would be great to validate the fix with the unit tests on CI with the problematic version. I'd suggest pushing a commit to temporarily pin matplotlib in pyproject.toml line 39:

[project.optional-dependencies]
plots = ["matplotlib==3.6.0", "ipython"]

(Nb we can ignore the test on python 3.7, which isn't compatible with matplotlib 3.6.0).

Once the other tests pass, we can then remove the version pin.

@jklaise
Copy link
Contributor Author

jklaise commented Jul 12, 2023 •
edited
Loading

FYI 3.6.1 removes the error and makes it a deprecation warning: https://matplotlib.org/stable/api/prev_api_changes/api_changes_3.6.1.html (however, I have not been able to see the deprecation warning they mention).

That being said, it's still worth adding explicit axes for people running 3.6.0 and also if the feature does get deprecated eventually.

Btw with the explicit axes we still get a warning No data for colormapping provided via 'c'. Parameters 'vmin', 'vmax' will be ignored, not sure at the moment if this is something we can ignore.

@connortann
Copy link
Collaborator

I think that the No data for colormapping provided warning is unrelated to the colobar issue. In any case, I'd like to poractively address all of the warnings in the test suite, before any of them become breaking changes. I created a separate issue to track them on #3066 , so we can tick them off over time.

@jklaise
Copy link
Contributor Author

jklaise commented Jul 12, 2023 •
edited
Loading

@connortann embedding_plot doesn't raise errors due to the colorbar, I think it's because it's called without arguments whilst in summary_plot the colorbar is created from a colormap.

The same should apply for monitoring_plot, but it seems this method doesn't actually work (and there are no uses of it in the docs or tests).

@connortann
Copy link
Collaborator

That's rather concerning, that we have no tests for monitoring_plot! Well, let's leave that for another day then.

If you'd kindly remove the temporary version pin, I think this PR is good to go!

@connortann connortann merged commit bf0adf8 into shap:master Jul 12, 2023
@connortann connortann modified the milestones: 0.43.0, 0.42.1 Jul 16, 2023
@kabartay
Copy link

Downgrading to 0.41.0 leads to this issue: #2909

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@connortann connortann connortann approved these changes

Assignees
No one assigned
Labels
bug Indicates an unexpected problem or unintended behaviour
Projects
None yet
Milestone
0.42.1
Development

Successfully merging this pull request may close these issues.

summary plot not working with matplotlib v3.6.0
3 participants
@jklaise @connortann @kabartay