-
-
Notifications
You must be signed in to change notification settings - Fork 18.5k
BUG: groupby.hist legend should use group keys #33493
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 10 commits
bab7491
e0c9466
61693c6
cbfc167
a5f5ba1
726d147
3e4925d
8db5247
a46609a
c36cae2
c01c7ab
c207ca2
a1a7e27
5b9cae7
1db3b35
0b0dbdd
0dba5d9
4f24547
1c9cef5
11cf3f8
c5749cb
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -225,6 +225,7 @@ def _grouped_hist( | |
xrot=None, | ||
ylabelsize=None, | ||
yrot=None, | ||
legend=False, | ||
**kwargs, | ||
): | ||
""" | ||
|
@@ -243,15 +244,27 @@ def _grouped_hist( | |
sharey : bool, default False | ||
rot : int, default 90 | ||
grid : bool, default True | ||
legend: : bool, default False | ||
kwargs : dict, keyword arguments passed to matplotlib.Axes.hist | ||
|
||
Returns | ||
------- | ||
collection of Matplotlib Axes | ||
""" | ||
|
||
if legend and "label" not in kwargs: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is there a reason why someone would pass There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this should just raise instead There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It seems to me that we would then be raising on the only case where label has any effect - or am I missing another case? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Also, I don't understand the reason why we would raise here when not raising makes it easier for the end user have different names appear in the legend. What does raising gain? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So my thought is that If a user wants particular labels they should just provide that to the grouper; adding this here is just another way of doing things so just API clout There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would completely agree if this was implemented within pandas. However it's being passed through to matplotlib via kwargs. Disabling certain kwargs from working and allowing others to go through seems hazardous to user expectations. But even if this is unconvincing, I think we should strive to be consistent with various plots:
Perhaps an issue could be raised as to how labels should be treated across all plots? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Well shoot. As a counter-argument to what I just said. DataFrame.plot.hist ignores label altogether. I suppose if there is one plotting function we should be consistent with here, it's that one. What do you think about not raising, but simply ignoring any label kwarg? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I still think we should raise instead of ignoring; ignoring leads to confusion or surprising behavior, so always better to be explicit |
||
if isinstance(data, ABCDataFrame): | ||
if column is None: | ||
kwargs["label"] = data.columns | ||
else: | ||
rhshadrach marked this conversation as resolved.
Show resolved
Hide resolved
|
||
kwargs["label"] = column | ||
else: | ||
kwargs["label"] = data.name | ||
rhshadrach marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
def plot_group(group, ax): | ||
ax.hist(group.dropna().values, bins=bins, **kwargs) | ||
if legend: | ||
ax.legend() | ||
|
||
if xrot is None: | ||
xrot = rot | ||
|
@@ -290,6 +303,7 @@ def hist_series( | |
yrot=None, | ||
figsize=None, | ||
bins=10, | ||
legend: bool = False, | ||
**kwds, | ||
): | ||
import matplotlib.pyplot as plt | ||
|
@@ -308,8 +322,11 @@ def hist_series( | |
elif ax.get_figure() != fig: | ||
raise AssertionError("passed axis not bound to passed figure") | ||
values = self.dropna().values | ||
|
||
if legend and "label" not in kwds: | ||
kwds["label"] = self.name | ||
ax.hist(values, bins=bins, **kwds) | ||
if legend: | ||
ax.legend() | ||
ax.grid(grid) | ||
axes = np.array([ax]) | ||
|
||
|
@@ -334,6 +351,7 @@ def hist_series( | |
xrot=xrot, | ||
ylabelsize=ylabelsize, | ||
yrot=yrot, | ||
legend=legend, | ||
**kwds, | ||
) | ||
|
||
|
@@ -358,6 +376,7 @@ def hist_frame( | |
figsize=None, | ||
layout=None, | ||
bins=10, | ||
legend: bool = False, | ||
**kwds, | ||
): | ||
if by is not None: | ||
|
@@ -376,6 +395,7 @@ def hist_frame( | |
xrot=xrot, | ||
ylabelsize=ylabelsize, | ||
yrot=yrot, | ||
legend=legend, | ||
**kwds, | ||
) | ||
return axes | ||
|
@@ -401,11 +421,17 @@ def hist_frame( | |
) | ||
_axes = _flatten(axes) | ||
|
||
can_set_label = "label" not in kwds | ||
|
||
for i, col in enumerate(data.columns): | ||
ax = _axes[i] | ||
if legend and can_set_label: | ||
kwds["label"] = col | ||
ax.hist(data[col].dropna().values, bins=bins, **kwds) | ||
ax.set_title(col) | ||
ax.grid(grid) | ||
if legend: | ||
ax.legend() | ||
|
||
_set_ticks_props( | ||
axes, xlabelsize=xlabelsize, xrot=xrot, ylabelsize=ylabelsize, yrot=yrot | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
DataFrameGroupBy and SeriesGroupBy aren't in the top-level. It should be something like
core.groupby.DataFrameGroupBy
. Seedoc/source/referenece.groupby.rst
for the right path..There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah - thanks!