Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

Add a neworientation parameter to Violinplot and deprecatevert#27998

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
timhoffm merged 4 commits intomatplotlib:mainfromsaranti:orientation
May 14, 2024

Conversation

saranti
Copy link
Contributor

@sarantisaranti commentedApr 1, 2024•
edited
Loading

PR summary

Partiallycloses#13435. Replace thevert: bool parameter withorientation : {'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

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.

saranti reacted with thumbs up emoji
Copy link
ContributorAuthor

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 putorientation in the place wherevert was?

timhoffm reacted with thumbs up emoji
@timhoffm
Copy link
Member

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

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

@timhoffm
Copy link
Member

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

@sarantisaranti marked this pull request as draftApril 3, 2024 09:06
@mwaskom
Copy link

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

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

timhoffm reacted with thumbs up emoji

@sarantisaranti marked this pull request as ready for reviewApril 3, 2024 10:38
@sarantisaranti marked this pull request as draftApril 4, 2024 07:40
@sarantisaranti marked this pull request as ready for reviewApril 4, 2024 08:35
@saranti
Copy link
ContributorAuthor

saranti commentedApr 5, 2024•
edited
Loading

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.

ifvertisnotNone:
_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.
ifvertorvertisNoneandorientationisNone:
orientation='vertical'
elifvertisFalse:
orientation='horizontal'

@sarantisaranti marked this pull request as ready for reviewApril 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 normalizedorientation 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 defaultorientation='vertical' instead of defaulting it to None?

saranti reacted with thumbs up emoji
Copy link
ContributorAuthor

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

@rcomerrcomer 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 theboxplot 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.

saranti reacted with thumbs up emoji
@@ -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

@rcomerrcomerMay 12, 2024•
edited
Loading

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 withdefault: None)

saranti reacted with thumbs up emoji
@timhoffmtimhoffm added this to thev3.10.0 milestoneMay 14, 2024
@timhoffmtimhoffm merged commit6e8fcbb intomatplotlib:mainMay 14, 2024
42 checks passed
@sarantisaranti deleted the orientation branchMay 15, 2024 07:26
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@QuLogicQuLogicQuLogic left review comments

@timhoffmtimhoffmtimhoffm approved these changes

@rcomerrcomerrcomer approved these changes

Assignees
No one assigned
Projects
None yet
Milestone
v3.10.0
Development

Successfully merging this pull request may close these issues.

boxplot/violinplot orientation-setting API
5 participants
@saranti@timhoffm@mwaskom@QuLogic@rcomer
[8]ページ先頭
©2009-2025 Movatter.jp