Make edgecolors and facecolors the same when using stepfilled histogram. #6494
Conversation
| for x, y, c in reversed(list(zip(xvals, yvals, color))): | ||
| split = 2 * len(bins) | ||
| split = 2 * len(bins) | ||
| for x, y, c in reversed(list(zip(xvals, yvals, reversed(color)))): |
There was a problem hiding this comment.
Can you remove the list()? It's unnecessary.
There was a problem hiding this comment.
No, it isn't. reversed() requires a sequence; zip in py3 returns an iterator.
There was a problem hiding this comment.
Oh, you're right. That's interesting; I guess it makes sense since zip doesn't supply __len__, but it's odd that something like dict.keys() won't work with it either.
There was a problem hiding this comment.
That's because py3 went wild over iterators, so dict.keys() and similar methods all return iterators. There is no __getitem__, so no way to reverse the order.
|
Hum, my patch also change the order of colors for when |
Carreau
force-pushed
the
stepfilled
branch
2 times, most recently
from
2238775
to
a01aad3
Compare
|
Yes, I think so, unless it fixed previously broken behavior. The logic of your patch is not obvious to me--but given the horrendous expanse of code in which it is embedded, that's not surprising. Does stepfilled even need both edgecolors and facecolors? If they are always going to be the same, I don't see any point having edgecolors--they can be 'none'. |
Carreau
force-pushed
the
stepfilled
branch
3 times, most recently
from
081f5fe
to
e927259
Compare
I believe with the edges the overall size of the bad will be bigger, I doubt it make much difference with default values, but with linewidth != defaultvalue, it will likely. It make some artifacts:
Which are less visible but still there with edges shown.
|
|
I think I got it, using classic style: Master: This patch:
|
Carreau
force-pushed
the
stepfilled
branch
2 times, most recently
from
b161807
to
c65e0ab
Compare
|
Test failures seem to be :
|
|
the |
| patches.append(self.fill( | ||
| x[:split], y[:split], | ||
| closed=True if fill else None, | ||
| facecolor=c, |
There was a problem hiding this comment.
Can you just use color = c in both cases?
There was a problem hiding this comment.
The black outlines on stepfilled seem odd to me, is that the behavior in 1.5.1 as well?
There was a problem hiding this comment.
No, because otherwise histype=stepfilled would have colored edges instead of black ones:
filled=None != filled=False
But, I thought so too in the beginning.
(Compare above screenshot with test file in the PR)
There was a problem hiding this comment.
I suspect this behavior is going to change in 2.x as I think edge color now defaults to face color rather than 'k' so just leave this as-is.
This one is happy now.
I went for |
Let me retry, but I believe so. |
| fig, axes = plt.subplots(nrows=2, ncols=4) | ||
| axes = axes.flatten() | ||
|
|
||
|
|
|
|
||
| for kg,_type,ax in zip(kwargs, types, axes): | ||
| ax.hist(x, n_bins, histtype=_type, stacked=True,**kg) | ||
| ax.set_title('%s/%s' % (kg, _type) ) |
|
Here is the test figure generated by 1.5.x - v1.5.2rc2-4-g7605dd4
Easy to spot, the color are wrong on the first ax.
kwarg I agree that this is confusing, but that's how it behaves. I'm not against a change, (especially if it simplify things) but it's not my decision to make. |
| ax.set_title('%s/%s' % (kg, _type) ) | ||
| ax.set_ylim(ymin=-50) | ||
|
|
||
| patches= axes[0].patches |
|
Sorry, I had to add a bunch of PEP8 comments because |
"a foolish consistency is the hobgoblin of little minds" says pep-8 :-) I've enabled it in my editor and fixed (a few) extra you missed. The file seem to be crippled with more pep8 issues (96 apparently) but I doubt it's the right time to fix'em'all. |
If you decide to change, I would gladly deprecate |
|
I think better to leave it as is for now, deprecating All of |
|
@Carreau Thanks! |
|
Thanks to you ! Happy 2.0b release ! |
Closes #6493