Skip to content

ENH: warn when silently ignoring log keywords in PiePlot #55890

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

Merged
merged 8 commits into from
Nov 13, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
72 changes: 52 additions & 20 deletions pandas/plotting/_matplotlib/core.py
Original file line number Diff line number Diff line change
Expand Up @@ -159,6 +159,14 @@ def __init__(
layout=None,
include_bool: bool = False,
column: IndexLabel | None = None,
*,
logx: bool | None | Literal["sym"] = False,
logy: bool | None | Literal["sym"] = False,
loglog: bool | None | Literal["sym"] = False,
mark_right: bool = True,
stacked: bool = False,
label: Hashable | None = None,
style=None,
**kwds,
) -> None:
import matplotlib.pyplot as plt
Expand Down Expand Up @@ -230,13 +238,13 @@ def __init__(
self.legend_handles: list[Artist] = []
self.legend_labels: list[Hashable] = []

self.logx = kwds.pop("logx", False)
self.logy = kwds.pop("logy", False)
self.loglog = kwds.pop("loglog", False)
self.label = kwds.pop("label", None)
self.style = kwds.pop("style", None)
self.mark_right = kwds.pop("mark_right", True)
self.stacked = kwds.pop("stacked", False)
self.logx = type(self)._validate_log_kwd("logx", logx)
Copy link
Member

Choose a reason for hiding this comment

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

Could we use a staticmethod so we could avoid this odd type(self).?

Copy link
Member Author

Choose a reason for hiding this comment

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

ive started using this pattern even with static/classmethods to make it easier to see when a method call is harmless state-wise

self.logy = type(self)._validate_log_kwd("logy", logy)
self.loglog = type(self)._validate_log_kwd("loglog", loglog)
self.label = label
self.style = style
self.mark_right = mark_right
self.stacked = stacked

# ax may be an Axes object or (if self.subplots) an ndarray of
# Axes objects
Expand Down Expand Up @@ -289,6 +297,22 @@ def _validate_sharex(sharex: bool | None, ax, by) -> bool:
raise TypeError("sharex must be a bool or None")
return bool(sharex)

@classmethod
def _validate_log_kwd(
cls,
kwd: str,
value: bool | None | Literal["sym"],
) -> bool | None | Literal["sym"]:
if (
value is None
or isinstance(value, bool)
or (isinstance(value, str) and value == "sym")
):
return value
raise ValueError(
f"keyword '{kwd}' should be bool, None, or 'sym', not '{value}'"
)

@final
@staticmethod
def _validate_subplots_kwarg(
Expand Down Expand Up @@ -555,14 +579,6 @@ def _axes_and_fig(self) -> tuple[Sequence[Axes], Figure]:

axes = flatten_axes(axes)

valid_log = {False, True, "sym", None}
input_log = {self.logx, self.logy, self.loglog}
if input_log - valid_log:
invalid_log = next(iter(input_log - valid_log))
raise ValueError(
f"Boolean, None and 'sym' are valid options, '{invalid_log}' is given."
)

if self.logx is True or self.loglog is True:
[a.set_xscale("log") for a in axes]
elif self.logx == "sym" or self.loglog == "sym":
Expand Down Expand Up @@ -1330,7 +1346,12 @@ def _make_plot(self, fig: Figure):
cbar.ax.set_yticklabels(self.data[c].cat.categories)

if label is not None:
self._append_legend_handles_labels(scatter, label)
self._append_legend_handles_labels(
# error: Argument 2 to "_append_legend_handles_labels" of
# "MPLPlot" has incompatible type "Hashable"; expected "str"
scatter,
label, # type: ignore[arg-type] # pyright: ignore[reportGeneralTypeIssues]
)

errors_x = self._get_errorbars(label=x, index=0, yerr=False)
errors_y = self._get_errorbars(label=y, index=0, xerr=False)
Expand Down Expand Up @@ -1993,10 +2014,21 @@ def __init__(self, data, kind=None, **kwargs) -> None:
if (data < 0).any().any():
raise ValueError(f"{self._kind} plot doesn't allow negative values")
MPLPlot.__init__(self, data, kind=kind, **kwargs)
self.grid = False
self.logy = False
self.logx = False
self.loglog = False

@classmethod
def _validate_log_kwd(
cls,
kwd: str,
value: bool | None | Literal["sym"],
) -> bool | None | Literal["sym"]:
super()._validate_log_kwd(kwd=kwd, value=value)
if value is not False:
warnings.warn(
f"PiePlot ignores the '{kwd}' keyword",
UserWarning,
stacklevel=find_stack_level(),
)
return False

def _validate_color_args(self) -> None:
pass
Expand Down
6 changes: 5 additions & 1 deletion pandas/tests/plotting/frame/test_frame.py
Original file line number Diff line number Diff line change
Expand Up @@ -333,10 +333,14 @@ def test_invalid_logscale(self, input_param):
# GH: 24867
df = DataFrame({"a": np.arange(100)}, index=np.arange(100))

msg = "Boolean, None and 'sym' are valid options, 'sm' is given."
msg = f"keyword '{input_param}' should be bool, None, or 'sym', not 'sm'"
with pytest.raises(ValueError, match=msg):
df.plot(**{input_param: "sm"})

msg = f"PiePlot ignores the '{input_param}' keyword"
with tm.assert_produces_warning(UserWarning, match=msg):
df.plot.pie(subplots=True, **{input_param: True})

def test_xcompat(self):
df = tm.makeTimeDataFrame()
ax = df.plot(x_compat=True)
Expand Down