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: Prototype new plotting API for bar plot, accept and return axes #3523

Merged
merged 14 commits into from Feb 28, 2024

Conversation

connortann
Copy link
Collaborator

@connortann connortann commented Feb 26, 2024

Overview

Supports #3411
Supports #3036

A prototype new standard API interface for the plots, using plots.bar as an example:

  • Add an ax parameter to accept an existing matplotlib axes
  • Return the axes (if show=False)
  • Internal refactor: use the explicit matplotlib interface rather than the imperative

Discussion

I do wonder if the show argument is really needed. In jupyter notebooks, I believe plt.show() is called anyway after each cell, so this seems a little redundant. However, it does have the nice effect of returning None which avoids cluttering the output with the __repr__ of the axes object.

I think changing the return type to ax from fig is technically a breaking change; but this was only a fairly recent addition, so it's probably not too significant.

Checklist

From #3411

  • Uses explicit object-oriented matplotlib interface
  • Accepts an optional ax parameter
  • Suitable tests using pytest-mpl
  • Docstring is complete and up-to-date
  • Notebook in the "API Examples" section runs and is up-to-date

@connortann connortann added the visualization Relating to plotting label Feb 26, 2024
@connortann connortann changed the title ENH: Add ax-level API to bar plot ENH: Prototype new plotting API for bar plot, accept and return axes Feb 26, 2024
@connortann connortann added BREAKING Indicates that a PR is introducing a breaking change enhancement Indicates new feature requests and removed BREAKING Indicates that a PR is introducing a breaking change labels Feb 26, 2024
Copy link

codecov bot commented Feb 27, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 60.65%. Comparing base (9c768ac) to head (3a7bb45).

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #3523      +/-   ##
==========================================
+ Coverage   59.42%   60.65%   +1.22%     
==========================================
  Files          90       90              
  Lines       12718    12722       +4     
==========================================
+ Hits         7558     7716     +158     
+ Misses       5160     5006     -154     

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

@connortann connortann marked this pull request as ready for review February 27, 2024 16:30
@connortann
Copy link
Collaborator Author

@DanGolding you may wish to take a skim through this PR if you get a minute

shap/plots/_bar.py Outdated Show resolved Hide resolved
@DanGolding
Copy link
Contributor

I do wonder if the show argument is really needed.

It seems unnecessary to me, unless this has become an established pattern in other libraries?

... avoids cluttering the output with the repr of the axes object

users can always end the line with a ; if they want to suppress the output being printed

@connortann
Copy link
Collaborator Author

connortann commented Feb 27, 2024

I added a proposed "checklist" for things that we wish to overhaul for each plot to #3411. I'll work through that for this PR as an example - in particular, we need more tests and to update the API example notebook.

Comment on lines +124 to +127
# we show the data on auto only when there are no transforms (excluding getitem calls)
if show_data == "auto":
show_data = len(op_history) == 0
transforms = [t for t in op_history if t.get("name") != "__getitem__"]
show_data = len(transforms) == 0
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Here is one functional change that I needed to make in order to reproduce more closely the plots in the API example notebook.

In the API example notebook, many plots have show_data=True. This adds the small grey numbers on the left axis. It looks like this behaviour was then changed at some point to only show data values when there are no transforms applied.

However, I think show_data still makes sense as True for a single explanation when the only operation applied has been __getitem__.

Before (show_data = False)

image

After (show_data = True)

image

@connortann
Copy link
Collaborator Author

Whilst we're at it, I updated the API examples notebook. Hopefully it should be executable on CI now. Reducing the sample size of the toy dataset from 32k to 2k rows sped up the runtime significantly, without changing the overall look or complexity of the example plots.

@CloseChoice CloseChoice added the BREAKING Indicates that a PR is introducing a breaking change label Feb 28, 2024
Copy link
Collaborator

@CloseChoice CloseChoice left a comment

Choose a reason for hiding this comment

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

LGTM. Just a couple doc nitpicks from me

shap/plots/_bar.py Show resolved Hide resolved
shap/plots/_bar.py Outdated Show resolved Hide resolved
shap/plots/_bar.py Outdated Show resolved Hide resolved
Co-authored-by: Tobias Pitters <31857876+CloseChoice@users.noreply.github.com>
@connortann
Copy link
Collaborator Author

Fab - I applied those changes, and shall merge shortly.

Regarding the show param: I agree we could theoretically remove it. I'm inclined to leave that for later, to avoid making too many breaking changes.

@connortann connortann merged commit d51d173 into master Feb 28, 2024
14 checks passed
@connortann connortann deleted the plots-api-prototype branch February 28, 2024 12:10
@connortann connortann added this to the 0.45.0 milestone Feb 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
BREAKING Indicates that a PR is introducing a breaking change enhancement Indicates new feature requests visualization Relating to plotting
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants