BUG-26214 fix colors parameter in DataFrame.boxplot #26456
Conversation
Codecov Report
@@ Coverage Diff @@ ## master #26456 +/- ## ========================================== - Coverage 91.73% 91.73% -0.01% ========================================== Files 174 174 Lines 50754 50762 +8 ========================================== + Hits 46560 46565 +5 - Misses 4194 4197 +3
Continue to review full report at Codecov.
|
Codecov Report
@@ Coverage Diff @@ ## master #26456 +/- ## ========================================= Coverage ? 91.86% ========================================= Files ? 179 Lines ? 50712 Branches ? 0 ========================================= Hits ? 46585 Misses ? 4127 Partials ? 0
Continue to review full report at Codecov.
|
doc/source/whatsnew/v0.25.0.rst Outdated
| - Bug in an error message in :meth:`DataFrame.plot`. Improved the error message if non-numerics are passed to :meth:`DataFrame.plot` (:issue:`25481`) | ||
| - Bug in incorrect ticklabel positions when plotting an index that are non-numeric / non-datetime (:issue:`7612` :issue:`15912` :issue:`22334`) | ||
| - | ||
| - Bug where :meth:`DataFrame.boxplot` would not accept a `color` parameter like `DataFrame.plot.box` (:issue:`26214`) |
There was a problem hiding this comment.
Does the behavior here match exactly what you'd get with DataFrame.plot.box? I assume the latter just passes kwargs on through but this looks to do special handling, so not entirely equivalent right?
There was a problem hiding this comment.
DataFrame.plot.box does special handling too. Neither color nor any of its valid keys are listed as a valid kwarg in matplotlib.pyplot.boxplot
| So does |
| Would be nice to wait until #26414 is in master to merge this. |
| we merged the refactor of the mpl backend, can you merge master and update to the new locations. |
| valid_keys = ['boxes', 'whiskers', 'medians', 'caps'] | ||
| for i in range(4): | ||
| if valid_keys[i] in colors: | ||
| result[i] = colors[valid_keys[i]] |
There was a problem hiding this comment.
Should invalid colors raise? To catch issues like .boxplot(colors={'boxse': 'red'})
There was a problem hiding this comment.
They did in DataFrame.plot.box, so yes, I think so
| @jreback I've merged master and updated the PR |
| | ||
| colors = kwds.pop('color', None) | ||
| if colors: | ||
| if isinstance(colors, dict): |
| @JustinZhengBC can you move whatsnew to 1.0.0 and merge master? |
| @WillAyd done |
WillAyd left a comment
WillAyd left a comment There was a problem hiding this comment.
Just so I'm clear - is the logic here the same for DataFrame.plot.box or do they diverge?
WillAyd left a comment
WillAyd left a comment There was a problem hiding this comment.
looks like a merge conflict in whatsnew but otherwise lgtm. @TomAugspurger maybe thoughts on this?
| @JustinZhengBC can you merge master and fix merge conflict? @TomAugspurger any thoughts on this? If not planning to merge in 2 days |
| Fixed the merge conflict. Merge on green. |
| Thanks @JustinZhengBC ! |
6 participants
git diff upstream/master -u -- "*.py" | flake8 --diffThis PR fixes issues in the handling of the
colorparameter in the implementation ofDataFrame.boxplotthat do not exist inDataFrame.plot.boxcolorparameter is now removed fromkwdsbefore thematplotlibcall, preventing an errorcolor, as_get_standard_colorsdid not preserve the appropriate order (without looking at that function, one can see that the previous implementation would give the boxes and the medians the same color even when otherwise specified)coloris actually adict