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

boxplot/violinplot orientation-setting API #13435

Closed
anntzer opened this issue Feb 14, 2019 · 11 comments · Fixed by #27998 or #28074
Closed

boxplot/violinplot orientation-setting API #13435

anntzer opened this issue Feb 14, 2019 · 11 comments · Fixed by #27998 or #28074

Comments

@anntzer
Copy link
Contributor

anntzer commented Feb 14, 2019

Bug report

Bug summary

Currently, boxplot(), bxp(), violin(), and violinplot() take a vert: bool argument to switch between vertical and horizontal; this is inconsistent with colorbar(), hist(), eventplot(), and Slider(), which take an orientation: {"vertical", "horizontal"} argument.

For consistency, I'd suggest adding support for orientation (both because I think colorbar() and hist(), in particular, are probably the most used among all these, and because I like the API better :-)), and later deprecate vert. Thoughts?

attn @phobson who has been involved in boxplots, IIRC.

@phobson
Copy link
Member

phobson commented Feb 14, 2019

I think this is a good idea.

Should be easy enough to capture vert, emit and deprecation warning, and then set orientation accordingly.

@anntzer
Copy link
Contributor Author

anntzer commented Feb 14, 2019

(and while we're at it, the manage_xticks arg should be renamed to manage_ticks, as it covers both orientations)

@phobson
Copy link
Member

phobson commented Feb 14, 2019

just throwing this out there: what would you say to manage_labels?

@anntzer
Copy link
Contributor Author

anntzer commented Feb 14, 2019

well technically it's managing ticks, tick labels, and axis limits so I think manage_labels is worse than manage_ticks, and perhaps manage_axis is better?
I also wonder whether it really needs to manage axis limits -- can't the standard margins system do that just as well? (needs to be investigated)

@phobson
Copy link
Member

phobson commented Feb 14, 2019

good points all around.

just noticed a wrinkle: boxplot.vert is an rcParam

@anntzer
Copy link
Contributor Author

anntzer commented Feb 14, 2019

We know how to handle deprecation/renaming of rcParams too, but thanks for pointing that out.

@github-actions
Copy link

This issue has been marked "inactive" because it has been 365 days since the last comment. If this issue is still present in recent Matplotlib releases, or the feature request is still wanted, please leave a comment and this label will be removed. If there are no updates in another 30 days, this issue will be automatically closed, but you are free to re-open or create a new issue if needed. We value issue reports, and this procedure is meant to help us resurface and prioritize issues that have not been addressed yet, not make them disappear. Thanks for your help!

@github-actions github-actions bot added the status: inactive Marked by the “Stale” Github Action label May 31, 2023
@anntzer anntzer added keep Items to be ignored by the “Stale” Github Action and removed status: inactive Marked by the “Stale” Github Action labels May 31, 2023
@anntzer
Copy link
Contributor Author

anntzer commented May 31, 2023

At least adding orientation should hopefully be uncontroversial.

@tacaswell tacaswell added this to the future releases milestone Jun 1, 2023
@saranti
Copy link
Contributor

saranti commented Mar 30, 2024

Would you want separate PRs for boxplot and violinplot to keep it simple? The review process for the boxplot section will probably take a bit longer because it involves the deprecation of an rcParam which Ive never done before.

@timhoffm
Copy link
Member

timhoffm commented Mar 30, 2024

Should we simply deprecate the boxplot.vert rcParams without replacement? This is more of a plot-type setting rather than style. It’s not useful as a global config or style parameter. The only use case would be a local context (e.g. via a context manager) if somebody wants to make multiple horizontal plots. But for that rare case using the parameter explicitly may be good enough and is more clear for the reader.

I believe, we also don’t have that config for bar() or hist().

@phobson
Copy link
Member

phobson commented Apr 3, 2024

Should we simply deprecate the boxplot.vert rcParams without replacement? [snip] I believe, we also don’t have that config for bar() or hist()

I think that's a good idea and the justification is solid, IMO

@QuLogic QuLogic modified the milestones: future releases, v3.10.0 May 15, 2024
@QuLogic QuLogic removed the keep Items to be ignored by the “Stale” Github Action label May 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants