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 side option to violinplot #27815

Merged
merged 1 commit into from
Mar 1, 2024

Conversation

anjabeck
Copy link

@anjabeck anjabeck commented Feb 23, 2024

PR summary

This PR adds a side parameter to the violin plot which allows to plot only one half of the violin.
I added an image comparison for vertical and horizontal violins and two subplots in the gallery example.

closes #27812

PR checklist

@anjabeck anjabeck marked this pull request as draft February 23, 2024 16:01
@anjabeck anjabeck changed the title Add side option to violinplot Draft: Add side option to violinplot Feb 23, 2024
Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

Thank you for opening your first PR into Matplotlib!

If you have not heard from us in a week or so, please leave a new comment below and that should bring it to our attention. Most of our reviewers are volunteers and sometimes things fall through the cracks.

You can also join us on gitter for real-time discussion.

For details on testing, writing docs, and our review process, please see the developer guide

We strive to be a welcoming and open project. Please follow our Code of Conduct.

@anjabeck anjabeck changed the title Draft: Add side option to violinplot Add side option to violinplot Feb 23, 2024
Copy link
Member

@ksunden ksunden left a comment

Choose a reason for hiding this comment

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

Fairly minor things:

Testing standard practices (using new style, random seed)

  • You were matching local examples, but we do have preferences for new tests
    Using helper function for validation.

As this is still in draft, didn't look too hard at the docs changes yet, but figured I'd point to some of our tooling early on.

lib/matplotlib/tests/test_axes.py Outdated Show resolved Hide resolved
lib/matplotlib/tests/test_axes.py Outdated Show resolved Hide resolved
lib/matplotlib/axes/_axes.py Show resolved Hide resolved
@anjabeck anjabeck marked this pull request as ready for review February 23, 2024 18:06
@ksunden ksunden dismissed their stale review February 23, 2024 18:07

comments addressed

@anjabeck
Copy link
Author

Can I fix the failed check without creating a new PR? (I changed the test image half-way through)

@ksunden
Copy link
Member

ksunden commented Feb 23, 2024

Yes, you can (and in fact opening a new PR alone would not even prevent the flag):

We have a guide on interactive rebasing here:

https://matplotlib.org/devdocs/devel/development_workflow.html#rewrite-commit-history

At minimum you'd need to squash together the two commits that added images. The easiest way to do that is probably to just squash all the commits down to one commit (its a small enough change for that)

If you do not want to mess with it yourself, we can do it as well.

@anjabeck anjabeck force-pushed the side-option-violinplot branch 2 times, most recently from 7a252c3 to dc97efb Compare February 23, 2024 18:44
@anjabeck
Copy link
Author

I tried myself, failed a little, but I think I managed to squash the commits.

Copy link
Member

@timhoffm timhoffm left a comment

Choose a reason for hiding this comment

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

As one can see in the image, there's a gap where extrema line and vertical line meet. We could investigate whether hard-coding capstyle="projecting" makes sense for extrema lines of half violins.

https://matplotlib.org/devdocs/gallery/lines_bars_and_markers/capstyle.html#capstyle

doc/users/next_whats_new/sides_violinplot.rst Outdated Show resolved Hide resolved
@anjabeck anjabeck force-pushed the side-option-violinplot branch 2 times, most recently from cd8ef8d to 50d2950 Compare February 28, 2024 13:34
add test for side option in violinplot

add whats new for side in violinplot

add violin side option to _axes.pyi and pyplot.py

use side option in violinplot also for lines

add side option to violinplot example

use newer style and correct seed in violin test

update image

change option naming

use proper syntax for code in violin docs

Co-authored-by: Kyle Sunden <git@ksunden.space>

add changed violinplots (capstyle=projecting)

hardcode capstyle only for left-right violins

fix typo
Copy link
Member

@timhoffm timhoffm left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks for the patience to go through all the review cycles!

@timhoffm timhoffm added this to the v3.9.0 milestone Mar 1, 2024
@anjabeck
Copy link
Author

anjabeck commented Mar 1, 2024

LGTM. Thanks for the patience to go through all the review cycles!

Thanks to you! This was very pleasant and I will definitely contribute to matplotlib in the future now that I know how you do things :)

@ksunden ksunden merged commit d57fa97 into matplotlib:main Mar 1, 2024
42 checks passed
@ksunden
Copy link
Member

ksunden commented Mar 1, 2024

Congrats on your merged PR! Thanks

@saranti saranti mentioned this pull request Mar 6, 2024
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[ENH]: Add split feature for violin plots
3 participants