Skip to content
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

Add 'color' and 'size' to arguments #44856

Merged
merged 24 commits into from Apr 7, 2022
Merged
Show file tree
Hide file tree
Changes from 9 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
44 changes: 42 additions & 2 deletions pandas/plotting/_core.py
Expand Up @@ -1127,6 +1127,16 @@ def bar(self, x=None, y=None, **kwargs):
axis of the plot shows the specific categories being compared, and the
other axis represents a measured value.
"""
c = kwargs.pop("c", None)
color = kwargs.pop("color", None)
if c is not None and color is not None:
raise TypeError("Specify exactly one of `c` and `color`")
if c is not None or color is not None:
if color is not None:
kwargs.setdefault("color", color)
else:
kwargs.setdefault("color", c)
attack68 marked this conversation as resolved.
Show resolved Hide resolved

return self(kind="bar", x=x, y=y, **kwargs)

@Appender(
Expand Down Expand Up @@ -1213,6 +1223,16 @@ def barh(self, x=None, y=None, **kwargs):
axis of the plot shows the specific categories being compared, and the
other axis represents a measured value.
"""
c = kwargs.pop("c", None)
color = kwargs.pop("color", None)
if c is not None and color is not None:
raise TypeError("Specify exactly one of `c` and `color`")
if c is not None or color is not None:
if color is not None:
kwargs.setdefault("color", color)
else:
kwargs.setdefault("color", c)
attack68 marked this conversation as resolved.
Show resolved Hide resolved

return self(kind="barh", x=x, y=y, **kwargs)

def box(self, by=None, **kwargs):
Expand Down Expand Up @@ -1582,7 +1602,7 @@ def pie(self, **kwargs):
raise ValueError("pie requires either y column or 'subplots=True'")
return self(kind="pie", **kwargs)

def scatter(self, x, y, s=None, c=None, **kwargs):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you can deprecate but not rename as that is a breaking change

these are matpotlib names but not objecting to the change per se

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see your point but I need some help to solve it. What is best practice in this situation and how does a deprication warning work?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mosc9575 check some other PRs which deprecate arguments for examples https://github.com/pandas-dev/pandas/pulls?q=is%3Apr+depr+is%3Amerged+

Copy link
Contributor Author

@mosc9575 mosc9575 Dec 12, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jreback I used the wrong word. I did not "rename", I added size and color to avoid braking code.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wouldn't keep both in the signature, that'd be confusing

I'd suggest:

  • use color and size in the signature
  • if color is None, take c from kwargs (likewise for size and s)
  • raise an error if someone passes both color and c
  • check that the docs consistently use color and size

Copy link
Contributor Author

@mosc9575 mosc9575 Dec 12, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@MarcoGorelli Thank you. I go with your suggestions. I will edit my PR.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks - will also need tests to cover the new behaviour

def scatter(self, x, y, **kwargs):
attack68 marked this conversation as resolved.
Show resolved Hide resolved
"""
Create a scatter plot with varying marker point size and color.

Expand Down Expand Up @@ -1665,7 +1685,27 @@ def scatter(self, x, y, s=None, c=None, **kwargs):
... c='species',
... colormap='viridis')
"""
return self(kind="scatter", x=x, y=y, s=s, c=c, **kwargs)
s = kwargs.pop("s", None)
attack68 marked this conversation as resolved.
Show resolved Hide resolved
size = kwargs.pop("size", None)
if s is not None and size is not None:
raise TypeError("Specify exactly one of `s` and `size`")
MarcoGorelli marked this conversation as resolved.
Show resolved Hide resolved
if s is not None or size is not None:
if s is not None:
kwargs.setdefault("s", s)
else:
kwargs.setdefault("s", size)
attack68 marked this conversation as resolved.
Show resolved Hide resolved

c = kwargs.pop("c", None)
attack68 marked this conversation as resolved.
Show resolved Hide resolved
color = kwargs.pop("color", None)
if c is not None and color is not None:
raise TypeError("Specify exactly one of `c` and `color`")
if c is not None or color is not None:
if c is not None:
kwargs.setdefault("c", c)
else:
kwargs.setdefault("c", color)
attack68 marked this conversation as resolved.
Show resolved Hide resolved

return self(kind="scatter", x=x, y=y, **kwargs)

def hexbin(self, x, y, C=None, reduce_C_function=None, gridsize=None, **kwargs):
"""
Expand Down
10 changes: 9 additions & 1 deletion pandas/tests/plotting/frame/test_frame.py
@@ -1,4 +1,3 @@
""" Test cases for DataFrame.plot """
from datetime import (
date,
datetime,
Expand Down Expand Up @@ -674,6 +673,11 @@ def test_plot_scatter(self):
with pytest.raises(TypeError, match=msg):
df.plot.scatter(y="y")

with pytest.raises(TypeError, match="Specify exactly one of `s` and `size`"):
df.plot.scatter(x="x", y="y", s=2, size=2)
with pytest.raises(TypeError, match="Specify exactly one of `c` and `color`"):
df.plot.scatter(x="a", y="b", c="red", color="green")

# GH 6951
axes = df.plot(x="x", y="y", kind="scatter", subplots=True)
self._check_axes_shape(axes, axes_num=1, layout=(1, 1))
Expand Down Expand Up @@ -827,6 +831,10 @@ def test_plot_bar(self):
_check_plot_works(df.plot.bar)

df = DataFrame({"a": [0, 1], "b": [1, 0]})

with pytest.raises(TypeError, match="Specify exactly one of `c` and `color`"):
df.plot.bar(x="a", y="b", c="red", color="green")

ax = _check_plot_works(df.plot.bar)
self._check_ticks_props(ax, xrot=90)

Expand Down
21 changes: 19 additions & 2 deletions pandas/tests/plotting/frame/test_frame_color.py
Expand Up @@ -123,6 +123,11 @@ def test_bar_colors(self):
self._check_colors(ax.patches[::5], facecolors=custom_colors)
tm.close()

custom_colors = "rgcby"
ax = df.plot.bar(c=custom_colors)
self._check_colors(ax.patches[::5], facecolors=custom_colors)
tm.close()

from matplotlib import cm

# Test str -> colormap functionality
Expand All @@ -141,6 +146,10 @@ def test_bar_colors(self):
self._check_colors([ax.patches[0]], facecolors=["DodgerBlue"])
tm.close()

ax = df.loc[:, [0]].plot.bar(c="DodgerBlue")
self._check_colors([ax.patches[0]], facecolors=["DodgerBlue"])
tm.close()

ax = df.plot(kind="bar", color="green")
self._check_colors(ax.patches[::5], facecolors=["green"] * 5)
tm.close()
Expand All @@ -151,14 +160,19 @@ def test_bar_user_colors(self):
)
# This should *only* work when `y` is specified, else
# we use one color per column
ax = df.plot.bar(y="A", color=df["color"])
result = [p.get_facecolor() for p in ax.patches]
expected = [
(1.0, 0.0, 0.0, 1.0),
(0.0, 0.0, 1.0, 1.0),
(0.0, 0.0, 1.0, 1.0),
(1.0, 0.0, 0.0, 1.0),
]

ax = df.plot.bar(y="A", color=df["color"])
result = [p.get_facecolor() for p in ax.patches]
assert result == expected

ax = df.plot.bar(y="A", c=df["color"])
result = [p.get_facecolor() for p in ax.patches]
mosc9575 marked this conversation as resolved.
Show resolved Hide resolved
assert result == expected

def test_if_scatterplot_colorbar_affects_xaxis_visibility(self):
Expand Down Expand Up @@ -223,6 +237,9 @@ def test_scatter_with_c_column_name_with_colors(self, cmap):
ax = df.plot.scatter(x=0, y=1, c="species", cmap=cmap)
assert ax.collections[0].colorbar is None

ax = df.plot.scatter(x=0, y=1, color="species", cmap=cmap)
assert ax.collections[0].colorbar is None

def test_scatter_colors(self):
df = DataFrame({"a": [1, 2, 3], "b": [1, 2, 3], "c": [1, 2, 3]})
with pytest.raises(TypeError, match="Specify exactly one of `c` and `color`"):
Expand Down
16 changes: 16 additions & 0 deletions pandas/tests/plotting/test_series.py
Expand Up @@ -270,14 +270,30 @@ def test_bar_ignore_index(self):

def test_bar_user_colors(self):
attack68 marked this conversation as resolved.
Show resolved Hide resolved
s = Series([1, 2, 3, 4])

expected = [
(1.0, 0.0, 0.0, 1.0),
(0.0, 0.0, 1.0, 1.0),
(0.0, 0.0, 1.0, 1.0),
(1.0, 0.0, 0.0, 1.0),
]

ax = s.plot.bar(color=["red", "blue", "blue", "red"])
result = [p.get_facecolor() for p in ax.patches]
assert result == expected

def test_bar_user_colors_c(self):
s = Series([1, 2, 3, 4])

expected = [
(1.0, 0.0, 0.0, 1.0),
(0.0, 0.0, 1.0, 1.0),
(0.0, 0.0, 1.0, 1.0),
(1.0, 0.0, 0.0, 1.0),
]

ax = s.plot.bar(c=["red", "blue", "blue", "red"])
result = [p.get_facecolor() for p in ax.patches]
attack68 marked this conversation as resolved.
Show resolved Hide resolved
assert result == expected

def test_rotation(self):
Expand Down