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: New plotting API for beeswarm so can accept and return Axes #3561
base: master
Are you sure you want to change the base?
Conversation
Tagging @connortann into this draft PR |
Thanks for the PR! The tests are currently failing due to an unrelated issue, which should be fixed shortly by#3563. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #3561 +/- ##
==========================================
+ Coverage 61.46% 61.47% +0.01%
==========================================
Files 90 90
Lines 12746 12753 +7
==========================================
+ Hits 7834 7840 +6
- Misses 4912 4913 +1 ☔ View full report in Codecov by Sentry. |
if show: | ||
pl.show() | ||
else: | ||
return pl.gca() | ||
pl.close(fig) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
May I ask about the motivation behind pl.close
? I think it may be a breaking change; we may as well decide a suitable pattern and plan to do that same for the other plots.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Of course. Jupyter will still render the plot even if show = false
. Closing the figure will prevent that. But maybe I've misunderstood what show is meant to be doing.
Ah if its a breaking change then perhaps more thought should be put into it. How do you think it's a breaking change?
This PR looks good to me! One suggestion - I think it would be preferable to raise a ValueError if plot_size and ax are both passed, rather than just ignoring the size. |
Good suggestion, I'll change it to that. |
Overview
Supports #3411
Change beeswarm to adhere to new standard API interface for the plots:
Discussion
The parameter
plot_size
is only really suitable for when there is one axes in the figure. If there are multiple then thedefault
value affects the figure size and not just the axes which the plot is being drawn to.We could use
plot_size
to just affect the size of the axes but this I think this is discouraged from mpl and can be quite tricky to do.So for now I thought it would be easier to just stop the behaviour of
plot_size
whenax
is not None. So if a user passes inax
into the plot then it is up to them to determine the size of the plot using the figure the axes has come from.We can definitely change this if it is not in line with the current thoughts on the new API.
Checklist