Thanks!

Can you please add a test for this? I think either an image test, e.g. with a number of subplots using different color arguments, or an image comparison test where one check that using the arguments and setting the colors afterwards result in identical images.

Edit: seehttps://matplotlib.org/stable/devel/testing.html#writing-an-image-comparison-test if more info on the testing is required.

I couldn't find a doc-page for that, but inhttps://github.com/matplotlib/matplotlib/tree/main/doc/users/next_whats_new there are small notes to be included in the next What's New. Please add one for this feature, ideally with an example showing how it works to highlight this new and useful feature.

Thanks!

Can you please add a test for this? I think either an image test, e.g. with a number of subplots using different color arguments, or an image comparison test where one check that using the arguments and setting the colors afterwards result in identical images.

Edit: seehttps://matplotlib.org/stable/devel/testing.html#writing-an-image-comparison-test if more info on the testing is required.

Test added, I have never written one but I believe this should work as expected.

And now there's a whats new page. I think that covers your requests. Let me know if there's anything else to do! Thanks for your feedback and support.

You might want to install the pre-commit hooks to help with the linting/formatting errors
of thehttps://matplotlib.org/devdocs/devel/development_setup.html#install-pre-commit-hooks

ETA: Also, thanks for jumping headfirst into this, this is a good feature to have.

I think that's everything@oscargus and@story645. Thanks for your help!

Maybe one should change both face- and edgecolor (to different values) in one of the subplots to confirm that it also works?

Good point. I added two new subplot axes to the test - one that changesfacecolor andedgecolor to different things, and one that checks ifcolor overwrites the other two.

I understand if you feel this is out of scope, but since you're already implementing these parameters how do you feel about implementing them vectorized? Meaning that you can also pass in a list of colors/facecolor/edgecolor like in bar?

Good idea! That's now implemented with documentation, a test for this feature, and an example for the gallery. It's implemented the same way as it is in thebar method, where a sequence of colors can be passed that has a different length than the data, but the violin method will repeat the colors.

@timhoffm I can tell you from my perspective as someone who thinks about matplotlib primarily as a user (this is my first PR) I'm very happy to have a plotting function that allows me to make something look good very quickly, but that has the option of more detailed customization if required. The way the violinplot works now (after this PR) seems to fit that criteria. You can make a violin or set of violins with a color scheme that works, in one line of code, then go and edit individual objects later if you need to.

--

For now the two minimal requirements if have on API consistency are

• Check thecolor behavior ofbar() and be consistent with it or argue why we should behave differently here.

The color behavior between the new violinplot() and bar() are identical in terms of how the colors are handled within the function. However, bar() only has edgecolor and color as inputs. I changed the violinplot() to have edgecolor, facecolor, and color (which overwrites the other two) in response to@ksunden's point in issue#27298. This makes it consistent with patches.

In my opinion, this makes a lot of sense, because a user might want specific control over the lines and patch within the violin plot (i.e. using edgecolor/facecolor), or might want to make everything look the same (i.e. using color). That seems like a perfect compromise between customizability and simplicity. So if anything, maybe bar() should copy this behavior?

Extra point: I think this edgecolor/facecolor/color schema additionally makes sense given the way I plan to handle alpha values, which I'll explain in the response to@ksunden and@story645 in the above thread.

--

• Isedgecolor the correct name? Does it color thebodies edges (if they have a finite width)? Should it?

I changed the name to edgecolor from linecolor because@ksunden and@story645 pointed out that that's the typical name for the rest of the library. I agree with that reasoning.

As it is, the bodies by default do not have an edgeline for violinplots. This is how it was before and still is. I think this is great, because it creates a very clean looking plot, and if the user wants to, they can go in and add an edge to the bodies. We can add this as an example to thecustomized violin plot in the gallery for the interested user if you think that's a good idea.

I guess I had it in my head when I made the original suggestion that the main shape were by default drawn with edges, which isn't actually true.

I think Tim is right thatbar is the closer analog here...

I do think that the most full solution would be to do something likebodyprops=dict(...),cmeanprops=dict(...), etc... (naming up to debate, as they are pluralized in the returns)... but I also agree that it is probably a bigger lift than wereally want.) That style is explicit and flexible (and something we do use in other parts of the library, particularly when there are multiple places**kwargs could be routed to, includingboxplot, which is the closest analog toviolinplot), but not necessarily the most friendly to use.

box hascolor (defaults to next patch color),edgecolor (defaults to None, which translates to the same as facecolor, I think) as well asecolor (defaults to black, used for error bars).

color is either a single color or a list of colors which is cycled.

Not to complicate things further but regarding the earlier discussion it also may be helpful to include an argument likefacealpha. I'm happy to keep working on it (although I think I may tap out if the lift tobodyprops=dict(...) is requested). But I think these decisions are above my pay grade so let me know what you all think.

Also, I'm not sure I understand why the tests failed.
The python 3.9 on ubuntu-20.04 (with and without Minimum Versions) failed because:
FAILED lib/matplotlib/tests/test_backends_interactive.py::test_figure_leak_20490[time_mem0-{'MPLBACKEND': 'wxagg'}]

And the Main Pytest Windows_py39 failed because:
FAILED lib/matplotlib/tests/test_pickle.py::test_pickle_load_from_subprocess[png]

But I'm not sure why that is happening or how it relates to the commits I've made.

it also may be helpful to include an argument like facealpha.

We added support for(color, alpha) as a validcolor specification precisely to support specifying alpha on specific components w/o growing kwargs#24691

how it relates to the commits I've made.

It shouldn't be directly related. While it is weird that it seems to happen consistently on this PR (and some others), there is no obvious reason why it happens.

it also may be helpful to include an argument like facealpha.

We added support for(color, alpha) as a validcolor specification precisely to support specifying alpha on specific components w/o growing kwargs#24691

Yep. I now removed the alpha handling so the alpha will just directly be controlled byfacecolor without defaults to alternative values.

I just wanted to ping the reviewers in case there is continued interest in this PR. I'm happy to continue working on the changes to the violinplot constructor, but there was no clear agreement on how best to manage the additional color arguments.

It seems to me like there is tension between which other plotting methodviolinplot should be consistent with -- should it be similar tobarplot orboxplot (or something else)? And -- should the kwargs be expanded to include parameter control over each feature of the violinplot -- e.g.bodies,cmeans,cmins, etc. or just have a single set of kwargs to control each feature of the plot.

In my opinion as a user, I'm very happy to have a few simple kwargs (like inbarplot, i.e. just havingcolor,edgecolor,facecolor) and letting matplotlib apply them to each feature of the plot so it looks good with minimal effort. The output of the constructor still provides the option of using the feature dictionaries for further specification if desired by a user.

@landoskape thanks for following up. We've got a developers call Thursday at I think 8:00PM UTC) where I think it might be good to discuss this - please join if you can make it:https://scientific-python.org/calendars/

I tend to agree w/@landoskape on keeping it simple: color/face/edge & if folks want more complicated then they can pop into the artist layer.

So looking at how seaborn just exposes body color,@mwaskom is one proposal (bar plot vs box plot) preferable?

seaborn.violinplot(data=None,*,x=None,y=None,hue=None,order=None,hue_order=None,orient=None,color=None,palette=None,saturation=0.75,fill=True,inner='box',split=False,width=0.8,dodge='auto',gap=0,linewidth=None,linecolor='auto',cut=2,gridsize=100,bw_method='scott',bw_adjust=1,density_norm='area',common_norm=False,hue_norm=None,formatter=None,log_scale=None,native_scale=False,legend='auto',scale=<deprecated>,scale_hue=<deprecated>,bw=<deprecated>,inner_kws=None,ax=None,**kwargs)

The code is good. Left some comments on documentation and tests.

I've added some minor formatting and code cleanup. See last commit.

Looks good, thanks for the cleanup! thatfacecol variable name was there from before I didn't have the heart to change it :)

@landoskape Thanks for the patience to bring this through!

Woot! Thanks for working with me on it. Glad it's part of matplotlib now

@timhoffm I was in the middle of reviewing this, should I just do a follow up PR?

@story645 sorry that was not obvious to me. Please make a followup PR.