From acd347323672b5b929e063b2a671f2b34b2148c1 Mon Sep 17 00:00:00 2001 From: Brock Date: Wed, 8 Nov 2023 18:36:09 -0800 Subject: [PATCH 1/4] ENH: warn when silently ignoring log keywords in PiePlot --- pandas/plotting/_matplotlib/core.py | 67 ++++++++++++++++------- pandas/tests/plotting/frame/test_frame.py | 6 +- 2 files changed, 52 insertions(+), 21 deletions(-) diff --git a/pandas/plotting/_matplotlib/core.py b/pandas/plotting/_matplotlib/core.py index b67a8186c8c2b..4b2a675d6cee9 100644 --- a/pandas/plotting/_matplotlib/core.py +++ b/pandas/plotting/_matplotlib/core.py @@ -157,6 +157,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 @@ -228,13 +236,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) + 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 self.ax = ax # TODO: deprecate fig keyword as it is ignored, not passed in tests @@ -283,6 +291,22 @@ def _validate_sharex(self, 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 def _validate_subplots_kwarg( self, subplots: bool | Sequence[Sequence[str]] @@ -534,14 +558,6 @@ def _setup_subplots(self) -> 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": @@ -1303,7 +1319,7 @@ def _make_plot(self, fig: Figure): plot_colorbar = self.colormap or c_is_column cb = self.kwds.pop("colorbar", is_numeric_dtype(c_values) and plot_colorbar) - if self.legend and hasattr(self, "label"): + if self.legend: label = self.label else: label = None @@ -1895,10 +1911,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 diff --git a/pandas/tests/plotting/frame/test_frame.py b/pandas/tests/plotting/frame/test_frame.py index 2ef1f065f603d..b57e57bc2dec0 100644 --- a/pandas/tests/plotting/frame/test_frame.py +++ b/pandas/tests/plotting/frame/test_frame.py @@ -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) From 0f9ddb9f391b708d79fe34b3b3f3ca34f88a0c92 Mon Sep 17 00:00:00 2001 From: Brock Date: Wed, 8 Nov 2023 19:30:44 -0800 Subject: [PATCH 2/4] pyright fixup --- pandas/plotting/_matplotlib/core.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/pandas/plotting/_matplotlib/core.py b/pandas/plotting/_matplotlib/core.py index 4b2a675d6cee9..d737e08c41533 100644 --- a/pandas/plotting/_matplotlib/core.py +++ b/pandas/plotting/_matplotlib/core.py @@ -1340,7 +1340,9 @@ 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( + scatter, label # pyright: ignore[reportGeneralTypeIssues] + ) else: self.legend = False From f6a49fa7d1a8453d7cbd638441ebe0cb9fe14496 Mon Sep 17 00:00:00 2001 From: Brock Date: Wed, 8 Nov 2023 20:22:34 -0800 Subject: [PATCH 3/4] mypy fixup --- pandas/plotting/_matplotlib/core.py | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/pandas/plotting/_matplotlib/core.py b/pandas/plotting/_matplotlib/core.py index d737e08c41533..232825d42a764 100644 --- a/pandas/plotting/_matplotlib/core.py +++ b/pandas/plotting/_matplotlib/core.py @@ -1341,7 +1341,10 @@ def _make_plot(self, fig: Figure): if label is not None: self._append_legend_handles_labels( - scatter, label # pyright: ignore[reportGeneralTypeIssues] + # error: Argument 2 to "_append_legend_handles_labels" of + # "MPLPlot" has incompatible type "Hashable"; expected "str" + scatter, + label, # pyright: ignore[reportGeneralTypeIssues] # type: ignore[arg-type] ) else: self.legend = False From 02468de0ee8aa2d235a758c107e2e42c7980a84e Mon Sep 17 00:00:00 2001 From: Brock Date: Thu, 9 Nov 2023 08:05:00 -0800 Subject: [PATCH 4/4] troubleshoot mypy --- pandas/plotting/_matplotlib/core.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pandas/plotting/_matplotlib/core.py b/pandas/plotting/_matplotlib/core.py index 232825d42a764..3f10620058868 100644 --- a/pandas/plotting/_matplotlib/core.py +++ b/pandas/plotting/_matplotlib/core.py @@ -1344,7 +1344,7 @@ def _make_plot(self, fig: Figure): # error: Argument 2 to "_append_legend_handles_labels" of # "MPLPlot" has incompatible type "Hashable"; expected "str" scatter, - label, # pyright: ignore[reportGeneralTypeIssues] # type: ignore[arg-type] + label, # type: ignore[arg-type] # pyright: ignore[reportGeneralTypeIssues] ) else: self.legend = False