|
| 1 | +# Issue #749: `pl.show()` ValueError — axes/panels count mismatch |
| 2 | + |
| 3 | +## Root cause |
| 4 | + |
| 5 | +When a SpatialElement carries transformations to **multiple coordinate systems** |
| 6 | +(e.g. visium's `"<cs>"` and `"<cs>_downscaled_lowres"` pair), `filter_by_coordinate_system` |
| 7 | +cannot strip the extra transformation (upstream spatialdata #176). `show()` then |
| 8 | +auto-detects **more coordinate systems than the user intended**. When the user |
| 9 | +passes a single `ax`, `_plan_panels` raises: |
| 10 | + |
| 11 | +``` |
| 12 | +ValueError: Mismatch between number of matplotlib axes objects (1) and number of panels (2). |
| 13 | +``` |
| 14 | + |
| 15 | +PR #580 added a `strict_cs` narrowing (keep only CS with element types for *all* |
| 16 | +render commands), but it does **not** help here: both coordinate systems contain |
| 17 | +the *same* element, so both survive the filter. The two panels are redundant — |
| 18 | +they render the identical element set, differing only by a scale transform. |
| 19 | + |
| 20 | +Confirmed reproducible on current `main` (see `/tmp/repro_issue_749.py`). |
| 21 | + |
| 22 | +## Proposed changes (recommended: Approach 1 — scope to the erroring path) |
| 23 | + |
| 24 | +Extend the existing `ax is not None and cs_was_auto` block in |
| 25 | +`_resolve_coordinate_systems` (`src/spatialdata_plot/pl/basic.py`): after the |
| 26 | +`strict_cs` step, if `len(coordinate_systems) > n_ax`, **deduplicate coordinate |
| 27 | +systems by their renderable-element set** (via `_get_elements_to_be_rendered`), |
| 28 | +keeping the first representative of each distinct set. If that brings the count |
| 29 | +down to `<= n_ax`, use the deduplicated list and emit a `UserWarning` naming the |
| 30 | +dropped (redundant) coordinate systems and pointing to `coordinate_systems=`. |
| 31 | +If the sets are genuinely distinct (count still `> n_ax`), fall through to the |
| 32 | +existing, correct `ValueError`. |
| 33 | + |
| 34 | +| File | Change | Rationale | |
| 35 | +|------|--------|-----------| |
| 36 | +| `src/spatialdata_plot/pl/basic.py` (`_resolve_coordinate_systems`) | After `strict_cs`, dedup redundant CS by element set when `ax` given + auto-detected; warn | Fixes the mismatch only on the path that currently errors → strictly backward compatible | |
| 37 | +| `tests/pl/test_show.py` | Regression test: multi-CS element + single `ax` no longer raises; distinct-CS case still raises | Lock behavior | |
| 38 | + |
| 39 | +Why scope to the `ax`-provided path only: the no-`ax` case currently produces |
| 40 | +one panel per coordinate system (redundant but not an error). Collapsing that |
| 41 | +too would change existing multi-panel output / baselines — a backward-compat |
| 42 | +break the reporter explicitly asked to avoid. The `ax` path currently *errors*, |
| 43 | +so fixing it breaks nothing that worked before. |
| 44 | + |
| 45 | +## Edge cases |
| 46 | +- [ ] 2 CS, same element set, single `ax` → collapse to 1, warn, render (main fix). |
| 47 | +- [ ] 2 CS, **different** element sets, single `ax` → still raise ValueError (correct; genuinely 2 panels). |
| 48 | +- [ ] N CS redundant, `ax` is a list of N → counts already match, no change. |
| 49 | +- [ ] N CS redundant, `ax` list shorter than distinct-set count → raise (correct). |
| 50 | +- [ ] `coordinate_systems=` passed explicitly → `cs_was_auto` False, block skipped, unchanged. |
| 51 | +- [ ] No `ax` → block skipped, unchanged (still multi-panel). |
| 52 | +- [ ] Multi-panel `color=[...]` → single CS required already; unaffected. |
| 53 | + |
| 54 | +## Downstream impact |
| 55 | +- Public API unchanged (no new/changed kwargs). |
| 56 | +- Only converts a previously-raised `ValueError` into a successful render + warning. |
| 57 | +- No baseline image changes expected (new path only exercised by the regression test, which is non-visual). |
| 58 | + |
| 59 | +## Test plan |
| 60 | +- [ ] Regression test (non-visual): multi-CS element, `filter_by_coordinate_system`, `render_shapes().show(ax=ax)` → no raise; assert 1 axis used. |
| 61 | +- [ ] Negative test: 2 CS with different elements + single `ax` → still raises `ValueError`. |
| 62 | +- [ ] Assert a `UserWarning` is emitted mentioning the dropped CS. |
| 63 | + |
| 64 | +## Risks |
| 65 | +- Representative choice is "first in coordinate-system order", which may be the |
| 66 | + `_downscaled_lowres` variant. Visually identical for a standalone plot (axes |
| 67 | + autoscale), but a user overlaying multiple `show()` calls on one `ax` should |
| 68 | + still pass `coordinate_systems=`. The warning makes the choice explicit. |
| 69 | +- Low risk overall: new code runs only where the code previously raised. |
| 70 | + |
| 71 | +## Alternative (Approach 2 — not recommended) |
| 72 | +Deduplicate redundant coordinate systems in auto-detect for **both** `ax` and |
| 73 | +no-`ax` paths. More internally consistent, but changes existing no-`ax` |
| 74 | +multi-panel output and possibly visual baselines → backward-incompatible. |
0 commit comments