From 4c3e5864a9d406f822fafbadf6d0958e396ab508 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 10 Sep 2026 09:15:01 +0000 Subject: [PATCH 1/2] fix(imshow): surface facet_row limitations instead of failing silently imshow with both facet_col and facet_row silently produced the wrong figure in two cases. On plotly < 6.7.0 (px.imshow has no facet_row), auto-assignment quietly dropped the slot, so a 4D array animated its fourth dimension instead of faceting subplot rows -- with no warning -- and a 5D array failed with an unrelated "Unassigned dimension(s)" error. The fallback now warns, naming the dimension and the plotly requirement, and the 5D case raises an error that explains the real cause. On any plotly version, px.imshow honours facet_col_wrap even when facet_row is set: the grid is built but the facet_row subplot titles are dropped. Every other px function ignores the wrap in that case, so drop it here too and warn. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01TKhPQzJgQr7khmzDPYAXA9 --- tests/test_accessor.py | 63 ++++++++++++++++++++++++++++++++- xarray_plotly/accessor.py | 6 ++-- xarray_plotly/plotting.py | 73 +++++++++++++++++++++++++++++++-------- 3 files changed, 125 insertions(+), 17 deletions(-) diff --git a/tests/test_accessor.py b/tests/test_accessor.py index aa8f1a4..3324f2f 100644 --- a/tests/test_accessor.py +++ b/tests/test_accessor.py @@ -498,12 +498,73 @@ def test_imshow_auto_skips_facet_row_on_old_plotly( ) -> None: """Test that auto-assignment skips facet_row on old plotly (4th dim animates).""" monkeypatch.setattr(plotting, "_imshow_supports_facet_row", lambda: False) - fig = self.da_4d.plotly.imshow() + with pytest.warns(UserWarning, match=r"'year' is animated instead of faceted"): + fig = self.da_4d.plotly.imshow() # year (4th dim) falls through to animation_frame instead of facet_row assert len(fig.frames) == 3 # only the facet_col (scenario) produces subplots assert len(fig.data) == 2 + def test_imshow_auto_facet_row_warning_mentions_plotly_version( + self, monkeypatch: pytest.MonkeyPatch + ) -> None: + """Test that the fallback warning points at the plotly requirement.""" + monkeypatch.setattr(plotting, "_imshow_supports_facet_row", lambda: False) + with pytest.warns(UserWarning, match=r"facet_row for imshow requires plotly>=6\.7\.0"): + self.da_4d.plotly.imshow() + + def test_imshow_no_animation_slot_left_on_old_plotly( + self, monkeypatch: pytest.MonkeyPatch + ) -> None: + """Test the 5D case on old plotly: no slot left to absorb facet_row.""" + monkeypatch.setattr(plotting, "_imshow_supports_facet_row", lambda: False) + da = xr.DataArray( + np.random.rand(2, 2, 2, 2, 2), + dims=["lat", "lon", "scenario", "year", "time"], + ) + with pytest.raises(ValueError, match=r"already fills the animation slot"): + da.plotly.imshow() + + @requires_imshow_facet_row + def test_imshow_facet_col_wrap_ignored_with_facet_row(self) -> None: + """Test that facet_col_wrap is dropped so facet_row titles survive.""" + with pytest.warns(UserWarning, match=r"facet_col_wrap is ignored"): + fig = self.da_4d.plotly.imshow(facet_col_wrap=2) + facet_titles = {a.text for a in fig.layout.annotations if "=" in (a.text or "")} + assert facet_titles == { + "scenario=low", + "scenario=high", + "year=2020", + "year=2021", + "year=2022", + } + + def test_imshow_facet_col_wrap_kept_without_facet_row(self) -> None: + """Test that facet_col_wrap still applies when there is no facet_row.""" + fig = self.da_3d.plotly.imshow(facet_col_wrap=2) + assert len(fig.data) == 3 + # Wrapping at 2 columns stacks the 3 facets over two rows of subplots. + domains = {tuple(fig.layout[k].domain) for k in fig.layout if k.startswith("yaxis")} + assert len(domains) == 2 + + @requires_imshow_facet_row + def test_imshow_facet_grid_places_data_in_right_subplot(self) -> None: + """Test that each (facet_col, facet_row) pair lands in its own subplot.""" + values = np.zeros((2, 3, 4, 5)) + for col in range(2): + for row in range(3): + values[col, row] = col * 10 + row + da = xr.DataArray( + values, + dims=["scenario", "year", "lat", "lon"], + coords={"scenario": ["low", "high"], "year": [2020, 2021, 2022]}, + ) + fig = da.plotly.imshow(x="lon", y="lat", facet_col="scenario", facet_row="year") + assert len(fig.data) == 6 + # Every subplot holds exactly one constant value, and all six differ. + constants = {float(np.unique(trace.z)[0]) for trace in fig.data} + assert constants == {0.0, 1.0, 2.0, 10.0, 11.0, 12.0} + class TestColorsParameter: """Tests for the unified colors parameter.""" diff --git a/xarray_plotly/accessor.py b/xarray_plotly/accessor.py index ec4fa50..cab0ed3 100644 --- a/xarray_plotly/accessor.py +++ b/xarray_plotly/accessor.py @@ -360,8 +360,10 @@ def imshow( y: Dimension for y-axis (rows). Default: first dimension. facet_col: Dimension for subplot columns. Default: third dimension. facet_row: Dimension for subplot rows. Default: fourth dimension. - Requires plotly>=6.7.0; on older versions this slot is skipped - during auto-assignment. + Requires plotly>=6.7.0; on older versions an auto-assigned + dimension animates instead (with a warning) and an explicitly + named one raises `ValueError`. `facet_col_wrap` is ignored + when `facet_row` is set. animation_frame: Dimension for animation. Default: fifth dimension. robust: If True, use 2nd/98th percentiles for color bounds (handles outliers). colors: Color scale name (e.g., "Viridis", "RdBu"). See module docs. diff --git a/xarray_plotly/plotting.py b/xarray_plotly/plotting.py index d52abce..967ccee 100644 --- a/xarray_plotly/plotting.py +++ b/xarray_plotly/plotting.py @@ -31,6 +31,8 @@ ) if TYPE_CHECKING: + from collections.abc import Hashable + import plotly.graph_objects as go from xarray import DataArray @@ -669,6 +671,45 @@ def _imshow_supports_facet_row() -> bool: return "facet_row" in inspect.signature(px.imshow).parameters +def _handle_unsupported_facet_row(slots: dict[str, Hashable], *, explicit: bool) -> None: + """Resolve an imshow ``facet_row`` slot that the installed plotly cannot draw. + + ``px.imshow`` gained ``facet_row`` in plotly 6.7.0. On older versions an + explicit request is an error, while an auto-assigned dimension falls back + to animating (with a warning, so the missing subplot rows are not a + silent surprise). If the animation slot is already taken there is nowhere + to fall back to, so that case raises as well. + + Args: + slots: Slot assignment from :func:`assign_slots` (mutated in place). + explicit: Whether the user named the ``facet_row`` dimension. + """ + import plotly + + dim = slots["facet_row"] + msg = f"facet_row for imshow requires plotly>=6.7.0 (installed: {plotly.__version__})." + + if explicit: + raise ValueError(msg) + + if slots.get("animation_frame") is not None: + msg = ( + f"{msg} Dimension {dim!r} cannot be faceted across subplot rows, and " + f"{slots['animation_frame']!r} already fills the animation slot. " + f"Upgrade plotly, or reduce a dimension with .sel(), .isel() or .mean()." + ) + raise ValueError(msg) + + warnings.warn( + f"{msg} Dimension {dim!r} is animated instead of faceted across " + f"subplot rows; upgrade plotly to facet it.", + UserWarning, + stacklevel=4, + ) + slots["animation_frame"] = dim + slots["facet_row"] = None + + def imshow( darray: DataArray, *, @@ -709,10 +750,11 @@ def imshow( Dimension for subplot columns. Default: third dimension. facet_row Dimension for subplot rows. Default: fourth dimension. - Requires plotly>=6.7.0; on older versions this slot is skipped - during auto-assignment (the fourth dimension animates instead). - Note: ``facet_col_wrap`` is ignored by plotly when ``facet_row`` - is set. + Requires plotly>=6.7.0; on older versions an auto-assigned + dimension animates instead and a ``UserWarning`` is emitted, while + an explicitly named one raises ``ValueError``. + Note: ``facet_col_wrap`` is ignored (with a warning) when + ``facet_row`` is set, matching the other plot types. animation_frame Dimension for animation. Default: fifth dimension. robust @@ -736,11 +778,6 @@ def imshow( """ px_kwargs = resolve_colors(colors, px_kwargs) - # On plotly < 6.7.0, px.imshow has no facet_row: skip auto-assignment so - # dimensions fall through to animation_frame instead. - if facet_row is auto and not _imshow_supports_facet_row(): - facet_row = None - slots = assign_slots( list(darray.dims), "imshow", @@ -751,14 +788,22 @@ def imshow( animation_frame=animation_frame, ) + if slots.get("facet_row") is not None and not _imshow_supports_facet_row(): + _handle_unsupported_facet_row(slots, explicit=facet_row is not auto) + facet_row_kwargs: dict[str, Any] = {} if slots.get("facet_row") is not None: - if not _imshow_supports_facet_row(): - import plotly - - msg = f"facet_row for imshow requires plotly>=6.7.0 (installed: {plotly.__version__})." - raise ValueError(msg) facet_row_kwargs["facet_row"] = slots["facet_row"] + # px.imshow honours facet_col_wrap even when facet_row is set, which + # builds the grid but silently drops the facet_row titles. Every other + # px function ignores the wrap in that case; match them. + if px_kwargs.pop("facet_col_wrap", None) is not None: + warnings.warn( + "facet_col_wrap is ignored when facet_row is set; " + "px.imshow would otherwise drop the facet_row subplot titles.", + UserWarning, + stacklevel=3, + ) # Transpose to: y (rows), x (cols), facet_col, facet_row, animation_frame transpose_order = [ From e9fa13ae3baf05334cb5352b70edc8d29e0024c8 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 10 Sep 2026 09:49:01 +0000 Subject: [PATCH 2/2] fix(imshow): validate slots so bad assignments raise a clear error Two slot assignments reached px.imshow and crashed inside its own slicing with an error that named nothing the caller had written: - The same dimension in two slots popped the same index twice, giving "IndexError: pop index out of range". - Facet and animation slots consuming all but one dimension left nothing for the second heatmap axis, giving "IndexError: list index out of range". Duplicates that reached the transpose first surfaced as "ValueError: axes don't match array" instead. Validate the assignment before transposing: every slot needs its own dimension, and both y and x must be filled. The error now names the dimension and the two slots that want it, or the slots that consumed the axes. Sweeping all 3125 combinations of auto/None/each dimension across the five slots of a 4D array: every one now either builds a figure or raises a ValueError that says what to change, with no change to the 1056 combinations that already worked. Also drop mypy's python_version = "3.10" pin, which failed CI on the 3.12 and 3.13 matrix entries -- numpy's stubs now use `type` statements that mypy rejects when targeting 3.10. Reproduced on origin/main with no other changes. Unpinned, mypy targets the interpreter it runs under, so each matrix entry checks its own version against the numpy stubs resolved for it. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01TKhPQzJgQr7khmzDPYAXA9 --- pyproject.toml | 4 +++- tests/test_accessor.py | 45 +++++++++++++++++++++++++++++++++++++++ xarray_plotly/plotting.py | 45 +++++++++++++++++++++++++++++++++++++++ 3 files changed, 93 insertions(+), 1 deletion(-) diff --git a/pyproject.toml b/pyproject.toml index 299232a..e43aea5 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -92,7 +92,9 @@ ignore = [ known-first-party = ["xarray_plotly"] [tool.mypy] -python_version = "3.10" +# No python_version pin: mypy then targets the interpreter it runs under, so +# each CI matrix entry checks its own version against the numpy stubs resolved +# for it. Pinning 3.10 made mypy reject numpy's `type` statements outright. strict = true warn_return_any = true warn_unused_ignores = true diff --git a/tests/test_accessor.py b/tests/test_accessor.py index 3324f2f..61bfbd6 100644 --- a/tests/test_accessor.py +++ b/tests/test_accessor.py @@ -547,6 +547,51 @@ def test_imshow_facet_col_wrap_kept_without_facet_row(self) -> None: domains = {tuple(fig.layout[k].domain) for k in fig.layout if k.startswith("yaxis")} assert len(domains) == 2 + def test_imshow_duplicate_dim_across_slots(self) -> None: + """Test a clear error when one dimension is asked to fill two slots. + + px.imshow otherwise dies with "IndexError: pop index out of range". + """ + with pytest.raises(ValueError, match=r"'scenario' is assigned to both"): + self.da_3d.plotly.imshow(y="lat", x="lon", facet_col="scenario", facet_row="scenario") + + def test_imshow_duplicate_dim_facet_row_and_animation(self) -> None: + """Test the duplicate check across facet_row and animation_frame.""" + with pytest.raises(ValueError, match=r"'year' is assigned to both"): + self.da_4d.plotly.imshow( + y="lat", x="lon", facet_col="scenario", facet_row="year", animation_frame="year" + ) + + def test_imshow_no_dimension_left_for_x(self) -> None: + """Test a clear error when facet/animation slots eat the heatmap axes. + + px.imshow otherwise dies with "IndexError: list index out of range". + """ + with pytest.raises(ValueError, match=r"needs a dimension for both 'y' and 'x'"): + self.da_4d.plotly.imshow(facet_col="lat", facet_row="lon", animation_frame="scenario") + + def test_imshow_2d_with_both_facets_leaves_no_axes(self) -> None: + """Test the error when a 2D array puts both of its dims into facets.""" + da = xr.DataArray( + np.random.rand(2, 3), dims=["a", "b"], coords={"a": [0, 1], "b": [0, 1, 2]} + ) + with pytest.raises(ValueError, match=r"needs a dimension for both 'y' and 'x'"): + da.plotly.imshow(facet_col="a", facet_row="b") + + @requires_imshow_facet_row + def test_imshow_explicit_x_y_facet_col_facet_row_4d(self) -> None: + """Test that naming all four slots on a 4D array builds the full grid.""" + fig = self.da_4d.plotly.imshow(x="lon", y="lat", facet_col="scenario", facet_row="year") + assert len(fig.data) == 6 + facet_titles = {a.text for a in fig.layout.annotations if "=" in (a.text or "")} + assert facet_titles == { + "scenario=low", + "scenario=high", + "year=2020", + "year=2021", + "year=2022", + } + @requires_imshow_facet_row def test_imshow_facet_grid_places_data_in_right_subplot(self) -> None: """Test that each (facet_col, facet_row) pair lands in its own subplot.""" diff --git a/xarray_plotly/plotting.py b/xarray_plotly/plotting.py index 967ccee..360bccb 100644 --- a/xarray_plotly/plotting.py +++ b/xarray_plotly/plotting.py @@ -671,6 +671,49 @@ def _imshow_supports_facet_row() -> bool: return "facet_row" in inspect.signature(px.imshow).parameters +_IMSHOW_SLOTS = ("y", "x", "facet_col", "facet_row", "animation_frame") + + +def _validate_imshow_slots(slots: dict[str, Hashable]) -> None: + """Check that imshow's slots form a usable heatmap before handing them to plotly. + + Every imshow slot is a separate axis of the data, so each needs its own + dimension and both heatmap axes must be filled. ``px.imshow`` does not + check either, and fails deep inside its own slicing with ``IndexError: + pop index out of range`` (a dimension used twice) or ``IndexError: list + index out of range`` (nothing left for y/x). + + Args: + slots: Slot assignment from :func:`assign_slots`. + + Raises: + ValueError: If a dimension fills two slots, or y/x is left empty. + """ + seen: dict[Hashable, str] = {} + for slot in _IMSHOW_SLOTS: + dim = slots.get(slot) + if dim is None: + continue + if dim in seen: + msg = ( + f"Dimension {dim!r} is assigned to both {seen[dim]!r} and {slot!r}. " + f"Each imshow slot needs its own dimension." + ) + raise ValueError(msg) + seen[dim] = slot + + missing = [slot for slot in ("y", "x") if slots.get(slot) is None] + if missing: + taken = {slot: dim for dim, slot in seen.items()} + msg = ( + f"imshow needs a dimension for both 'y' and 'x', but {missing} " + f"came up empty; the other slots took {taken}. Free one with " + f"facet_col=None, facet_row=None or animation_frame=None, or reduce " + f"a dimension with .sel(), .isel() or .mean() before plotting." + ) + raise ValueError(msg) + + def _handle_unsupported_facet_row(slots: dict[str, Hashable], *, explicit: bool) -> None: """Resolve an imshow ``facet_row`` slot that the installed plotly cannot draw. @@ -788,6 +831,8 @@ def imshow( animation_frame=animation_frame, ) + _validate_imshow_slots(slots) + if slots.get("facet_row") is not None and not _imshow_supports_facet_row(): _handle_unsupported_facet_row(slots, explicit=facet_row is not auto)