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

Add a new orientation parameter to Violinplot and deprecate vert #27998

Merged
merged 4 commits into from May 14, 2024

Conversation

saranti
Copy link
Contributor

@saranti saranti commented Apr 1, 2024

PR summary

Partially closes #13435. Replace the vert: bool parameter with orientation : {'vertical', 'horizontal'}.
Also changed the Violin plot basics gallery example to reflect the changes.

The other half of issue #13435 is to do the same to boxplots

PR checklist

lib/matplotlib/axes/_axes.pyi Outdated Show resolved Hide resolved
lib/matplotlib/axes/_axes.pyi Outdated Show resolved Hide resolved
Comment on lines 8301 to 8304
def violinplot(self, dataset, positions=None, vert=None,
orientation='vertical', widths=0.5, showmeans=False,
showextrema=True, showmedians=False,
quantiles=None, points=100, bw_method=None, side='both'):
Copy link
Member

Choose a reason for hiding this comment

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

orientation can't be inserted in the middle here to prevent breaking passing all arguments positionally. I believe that will be fixed by #27786.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since #27786 is now merged, should we put orientation in the place where vert was?

lib/matplotlib/axes/_axes.py Outdated Show resolved Hide resolved
lib/matplotlib/axes/_axes.py Outdated Show resolved Hide resolved
lib/matplotlib/axes/_axes.py Outdated Show resolved Hide resolved
lib/matplotlib/axes/_axes.py Outdated Show resolved Hide resolved
lib/matplotlib/axes/_axes.py Show resolved Hide resolved
lib/matplotlib/axes/_axes.py Outdated Show resolved Hide resolved
lib/matplotlib/axes/_axes.py Outdated Show resolved Hide resolved
@timhoffm
Copy link
Member

timhoffm commented Apr 2, 2024

It would also be nice to make this a pending deprecation at first. Otherwise users of downstream libraries like seaborn (attn @mwaskom) will see our deprecation warning, but cannot do anything about it because it's in the seaborn code base. During the pending period, downstream libraries can already switch to the new API.

@mwaskom
Copy link

mwaskom commented Apr 3, 2024

Thoughtful but no need specifically for seaborn — seaborn's violinplots predate matplotlib's by several years :D

@timhoffm
Copy link
Member

timhoffm commented Apr 3, 2024

Thanks for the feedback. I’ve only checked boxplot, which will get the same change ( #13435).

@saranti saranti marked this pull request as draft April 3, 2024 09:06
@mwaskom
Copy link

mwaskom commented Apr 3, 2024

Oh actually boxplot is relevant to me. But I don’t think this is right:

It would also be nice to make this a pending deprecation at first. Otherwise users of downstream libraries like seaborn (attn @mwaskom) will see our deprecation warning

The DeprecationWarning class is fine; by default it only shows up in tests or when raised in “main”. The issue is when libraries jump straight to FutureWarning, which is always visible.

@saranti saranti marked this pull request as ready for review April 3, 2024 10:38
@saranti saranti marked this pull request as draft April 4, 2024 07:40
@saranti saranti marked this pull request as ready for review April 4, 2024 08:35
@saranti
Copy link
Contributor Author

saranti commented Apr 5, 2024

Edit: This block of code is going to be copied verbatim for the boxplots. Would it be better to turn it into a function and add it to _axes.py or cbook?
It's only needed during the deprecation period. Once it's over, the whole thing will be removed.

if vert is not None:
_api.warn_deprecated(
"3.10",
name="vert: bool",
alternative="orientation: {'vertical', 'horizontal'}"
)
# vert and orientation parameters are linked until vert's
# deprecation period expires. If both are selected,
# vert takes precedence.
if vert or vert is None and orientation is None:
orientation = 'vertical'
elif vert is False:
orientation = 'horizontal'

@saranti saranti marked this pull request as ready for review April 17, 2024 11:37
Comment on lines 8609 to 8618
# vert and orientation parameters are linked until vert's
# deprecation period expires. If both are selected,
# vert takes precedence.
if vert or vert is None and orientation is None:
orientation = 'vertical'
elif vert is False:
orientation = 'horizontal'

if orientation is not None:
_api.check_in_list(['horizontal', 'vertical'], orientation=orientation)
Copy link
Member

Choose a reason for hiding this comment

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

I'm not following the logic here completely. After whatever mangling vert and orientation, shouldn't we have a normalized orientation that must be 'horizontal' or 'vertical'? In other words

  • Why do we allow orientation to be None in 8617 and would that even work for the following code?

Can't we simply

if vert is not None:
    _api.warn_deprecated(...)
    orientation = 'vertical' if vert else 'horizontal'
_api.check_in_list(['horizontal', 'vertical'], orientation=orientation)

and default orientation='vertical' instead of defaulting it to None?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes it should default to vertical instead of None (it even says so in the docstring) so there was no reason for it other than making it more convoluted. That code change is great, I'll add it to the next commit

Copy link
Member

@rcomer rcomer left a comment

Choose a reason for hiding this comment

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

This seems much simpler than the boxplot one!

This block of code is going to be copied verbatim for the boxplots. Would it be better to turn it into a function and add it to _axes.py or cbook?
It's only needed during the deprecation period. Once it's over, the whole thing will be removed.

This is a good point. I think my inclination is to leave it duplicated in the two functions. It's pretty simple (more simple now than when you asked the question!) so I don't think the duplication does much harm.

@@ -8371,8 +8372,20 @@ def violinplot(self, dataset, positions=None, vert=True, widths=0.5,
vertical violins (or y-axis for horizontal violins).

vert : bool, default: True.
Copy link
Member

@rcomer rcomer May 12, 2024

Choose a reason for hiding this comment

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

Suggested change
vert : bool, default: True.
vert : bool, optional

(optional is synonymous with default: None)

@timhoffm timhoffm added this to the v3.10.0 milestone May 14, 2024
@timhoffm timhoffm merged commit 6e8fcbb into matplotlib:main May 14, 2024
42 checks passed
@saranti saranti deleted the orientation branch May 15, 2024 07:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

boxplot/violinplot orientation-setting API
5 participants