Allow scalar height for plt.bar #7644
Conversation
|
|
||
| @cleanup | ||
| def test_bar_single_height(): | ||
| # Check that a bar chart with a single height for all bars works |
There was a problem hiding this comment.
Slightly better to do
fig, ax = plt.subplots()
ax.bar(...)which helps to protect from tests leaking into each other a bit more.
| @@ -2025,6 +2025,8 @@ def make_iterable(x): | |||
| bottom = [0] | |||
|
|
|||
| nbars = len(left) | |||
| if len(height) == 1: | |||
There was a problem hiding this comment.
These 6 lines of code could be factored out of the if statement. Only nbars need to be inside the if - elif statement.
There was a problem hiding this comment.
Good spot - 4 of them can (I've done that below), but one of the if statements has to stay I think (as it's different for vertical or horizontal orientations)
|
@dstansby Can you rebase ? It seems there is a conflict between your branch and master. |
8e165d7 to
70afe12
Compare
|
Done! |
Current coverage is 62.11% (diff: 100%)@@ master #7644 diff @@
==========================================
Files 174 174
Lines 56022 56022
Methods 0 0
Messages 0 0
Branches 0 0
==========================================
- Hits 34799 34797 -2
- Misses 21223 21225 +2
Partials 0 0
|
|
|
||
| tick_label_axis = self.yaxis | ||
| tick_label_position = bottom | ||
| else: | ||
| raise ValueError('invalid orientation: %s' % orientation) | ||
|
|
||
| if len(height) == 1: | ||
| height *= nbars |
There was a problem hiding this comment.
It looks like this is leaving in place an old bug--probably harmless, since no one seems to have tripped over it. The potential problem is that if a kwarg like 'height' is passed in as an ndarray of length 1, it will arrive here and have its value multiplied by nbars, which might not be 1.
_make_iterable could be modified to ensure that if an argument comes in with length 1, it is returned as a list; or it could be turned into a _to_list, and turn any argument into a list. The point is that in at least parts of the code the returned object is assumed to be a list, not just an iterable.
I think the _to_list approach is the way to go; for these variables, there is no advantage in leaving their type as an indeterminate 'iterable'.
|
Now also addressed by #7562 (#7562 (comment)), which also covers the issue mentioned by @efiring. |
|
Great, if/when that gets merged feel free to close this PR. |
|
Actually I noticed that my PR doesn't include the docstring update, so you can merge this one and let my PR just overwrite the fix with another fix... |
|
To check I understand:
|
|
Yes on my side. |
Fixes #6820