FIX: Keep legacy alpha behavior for violinplot without facecolor #30636
Conversation
timhoffm
force-pushed
the
fix-violin-artist-alpha
branch
from
3c87254 to
520c6c7
Compare
lib/matplotlib/axes/_axes.py
Outdated
| You can use the ``(color, alpha)`` tuple notation to pass | ||
| semi-transparent colors. |
There was a problem hiding this comment.
Little unclear here, do you mean that (color, alpha) supercedes but does not set the artist alpha? So for example linecolor still used the artist alpha?
There was a problem hiding this comment.
Changed to
To set transparency for facecolor or edgecolor use
(color, alpha)tuples.
timhoffm
force-pushed
the
fix-violin-artist-alpha
branch
from
520c6c7 to
f70a4a2
Compare
timhoffm
added
the
Release critical
|
Test failure is our flaky timeout and unrelated to the PR. |
story645
left a comment
story645
left a comment
There was a problem hiding this comment.
I'm not clear on why the test sets a facecolor via if statement but otherwise looks good. Thanks for taking care of this.
| # Case 1: If facecolor is unspecified, it's the first color from the color cycle | ||
| # with Artist-level alpha=0.3 | ||
| facecolor = ('y' if mpl.rcParams['_internal.classic_mode'] | ||
| else plt.rcParams['axes.prop_cycle'].by_key()['color'][0]) |
There was a problem hiding this comment.
why is the color controlled by if statement, rather than fixed, given that this is a test?
There was a problem hiding this comment.
when debugging this I ran local code snippets until it worked. Then , I turned them into a test, which failed because our tests still use the classic style.
I figured it is reasonable to make the test robust to work with any style even though we are only testing one. It may be that we switch to using our default theme for testing at some point in the future. And it would be a nuisance if that test then falls over and someone has to figure out why.
There was a problem hiding this comment.
it is reasonable to make the test robust to work with any style even though we are only testing one
I think that's what paramaterization is for and that tests should always have the things being tested fixed, but since this is release critical you're welcome to merge.
There was a problem hiding this comment.
We typically do not test multiple styles. And also here, the tested logic - i.e. the alpha handling - is not style dependent. It's only that the test boundary conditions - i.e. the default facecolor - are style dependent. You could work around this by doing a first plot without alpha and infer the facecolor from that, but I figure it's simpler to just hard-code the two cases, even if they are not both exercised in the tests.
3 participants
Closes #30613.