Skip to content

Commit 8001f9e

Browse files
timtreisclaude
andcommitted
Fix review issues: reject norm list without cmap list, scope to images only
- Use resolved per-element cmap for list branching so norm list works correctly when cmap is auto-replicated - Raise ValueError when norm is a list but cmap is not (no silent pass-through to _prepare_cmap_norm) - Restrict list norm validation to element_type="images" only — labels has no list-norm support - Add test for norm list without explicit cmap list Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
1 parent c764384 commit 8001f9e

3 files changed

Lines changed: 23 additions & 7 deletions

File tree

src/spatialdata_plot/pl/basic.py

Lines changed: 13 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -632,27 +632,34 @@ def render_images(
632632

633633
for element, param_values in params_dict.items():
634634
cmap_params: list[CmapParams] | CmapParams
635-
if isinstance(cmap, list):
635+
resolved_cmap = param_values.get("cmap") or cmap
636+
if isinstance(resolved_cmap, list):
636637
if isinstance(norm, list):
637-
if len(norm) != len(cmap):
638+
if len(norm) != len(resolved_cmap):
638639
raise ValueError(
639-
f"Length of 'norm' list ({len(norm)}) must match the number of colormaps ({len(cmap)})."
640+
f"Length of 'norm' list ({len(norm)}) must match "
641+
f"the number of colormaps ({len(resolved_cmap)})."
640642
)
641643
norms = norm
642644
else:
643-
norms = [norm] * len(cmap)
645+
norms = [norm] * len(resolved_cmap)
644646
cmap_params = [
645647
_prepare_cmap_norm(
646648
cmap=c,
647649
norm=n,
648650
na_color=param_values["na_color"],
649651
)
650-
for c, n in zip(cmap, norms, strict=True)
652+
for c, n in zip(resolved_cmap, norms, strict=True)
651653
]
652654

653655
else:
656+
if isinstance(norm, list):
657+
raise ValueError(
658+
"Parameter 'norm' can only be a list when multiple colormaps are used. "
659+
"Pass a list of colormaps via 'cmap' or use a single Normalize."
660+
)
654661
cmap_params = _prepare_cmap_norm(
655-
cmap=cmap,
662+
cmap=resolved_cmap,
656663
norm=norm,
657664
na_color=param_values["na_color"],
658665
**kwargs,

src/spatialdata_plot/pl/utils.py

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2409,14 +2409,16 @@ def _type_check_params(param_dict: dict[str, Any], element_type: str) -> dict[st
24092409

24102410
norm = param_dict.get("norm")
24112411
if norm is not None:
2412-
if element_type in {"images", "labels"}:
2412+
if element_type == "images":
24132413
if isinstance(norm, list):
24142414
if not norm:
24152415
raise ValueError("Parameter 'norm' list must not be empty.")
24162416
if not all(isinstance(n, Normalize) for n in norm):
24172417
raise TypeError("Every item in 'norm' list must be a Normalize instance.")
24182418
elif not isinstance(norm, Normalize):
24192419
raise TypeError("Parameter 'norm' must be a Normalize or a list of Normalize instances.")
2420+
elif element_type == "labels" and not isinstance(norm, Normalize):
2421+
raise TypeError("Parameter 'norm' must be of type Normalize.")
24202422
if element_type in {"shapes", "points"} and not isinstance(norm, bool | Normalize):
24212423
raise TypeError("Parameter 'norm' must be a boolean or a mpl.Normalize.")
24222424

tests/pl/test_render_images.py

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -473,3 +473,10 @@ def test_norm_list_with_invalid_element_raises():
473473
sdata = _make_multichannel_sdata()
474474
with pytest.raises(TypeError, match="Normalize instance"):
475475
sdata.pl.render_images("img", norm=["not_a_norm"]).pl.show()
476+
477+
478+
def test_norm_list_without_cmap_list_raises():
479+
"""Norm list requires explicit cmap list."""
480+
sdata = _make_multichannel_sdata()
481+
with pytest.raises(ValueError, match="can only be a list when multiple colormaps"):
482+
sdata.pl.render_images("img", norm=[Normalize(0, 1)] * 3).pl.show()

0 commit comments

Comments
 (0)