Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

fix: heatmap plot colorbar and internal refactoring #3040

Merged
merged 11 commits into from
Jun 26, 2023

Conversation

thatlittleboy
Copy link
Collaborator

@thatlittleboy thatlittleboy commented Jun 24, 2023

Overview

Description of the changes proposed in this pull request:

  • Fixed the colorbar aspect ratio in shap.plots.heatmap, see:
    image
  • Internally, I've switched to an ax-based implementation instead of plt.imshow(), plt.barh() etc.; primarily so we don't have to do pl.gca() all over the place, and also the axes is needed to pass into plt.colorbar() to avoid warnings about
       Unable to determine Axes to steal space for Colorbar. Using gca(), but will raise in the future. Either provide the *cax* argument to use as the Axes for the Colorbar, provide the *ax* argument to steal space from it, or add *mappable* to an Axes.
    
  • The keen eye might notice that in the baseline images above, the tick directions have swapped from "in" to "out" -> note that the tick directions being "in" in the original baseline image is just a result of pytest-mpl using the "classic" matplotlib style; anybody with a more recent matplotlib version should already have the tick directions being "out" even prior to this PR. I only made this explicit in-code so that the baseline images generated by pytest-mpl reflects what most users would get by default ("out" tick directions).
  • Render the f(x) text in MathTex rather than plain text to match with the force plots.
  • Added comments and grouped similar code (e.g. the tick/axes manipulation code) together for better code readability.

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)

@codecov
Copy link

codecov bot commented Jun 24, 2023

Codecov Report

Merging #3040 (3745d13) into master (198dcb6) will not change coverage.
The diff coverage is 0.00%.

@@          Coverage Diff           @@
##           master   #3040   +/-   ##
======================================
  Coverage    0.00%   0.00%           
======================================
  Files          90      90           
  Lines       12855   12850    -5     
======================================
+ Misses      12855   12850    -5     
Impacted Files Coverage Δ
shap/plots/_heatmap.py 0.00% <0.00%> (ø)

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

@thatlittleboy thatlittleboy marked this pull request as ready for review June 25, 2023 10:03
@thatlittleboy thatlittleboy added this to the 2023-06-fixes milestone Jun 25, 2023
Copy link
Collaborator

@connortann connortann left a comment

Choose a reason for hiding this comment

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

LGTM! There are several notices from Codecov about patches not covered by tests, as the code coverage is not currently being reported correctly. I'll see if I can look in to that tomorrow.

@thatlittleboy
Copy link
Collaborator Author

Failing tests are from pytorch mnist cnn, irrelevant to the changes in this PR. See #2960

@thatlittleboy thatlittleboy merged commit 2797db5 into master Jun 26, 2023
@thatlittleboy thatlittleboy deleted the refactor-heatmap branch June 26, 2023 05:56
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants