Skip to content

Commit a5dfd13

Browse files
committed
Fix misleading cmap errors when norm/palette interact (#622)
Three related defects all manifested as errors blaming `cmap` for a `norm`/`palette` mistake the user never made: 1. `norm=[n1, n2]` for a 3-channel image raised "If 'cmap' is provided, its length must match the number of channels." even though the user only passed `norm=`. _validate_image_render_params now validates the norm-list length against the channel count up front, with a norm-specific error message. 2. `cmap=["Reds","Greens"]` (wrong length) combined with `norm=` of correct length silently nulled the cmap and used the default. The wrong-length cmap is now rejected uniformly, matching the no-norm path. 3. `palette + norm=list` raised "If 'palette' is provided, 'cmap' must be None." because basic.py auto-expanded a default-cmap list to carry the per-channel norms, and render.py couldn't tell the synthesized list apart from a user-supplied one. The render path now keys the palette/cmap conflict on whether any CmapParams was actually user-supplied (cmap_is_default=False); the synthesized default-cmap list falls through to the palette path, where per-channel norms are already applied to the layers. The "blending multiple cmaps" warning is also gated on user-supplied cmaps, so it no longer fires spuriously when the user only set `norm` or `palette + norm=list`.
1 parent e107c0a commit a5dfd13

3 files changed

Lines changed: 60 additions & 13 deletions

File tree

src/spatialdata_plot/pl/render.py

Lines changed: 18 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -1286,9 +1286,20 @@ def _render_images(
12861286
),
12871287
)
12881288

1289-
# True if user gave n cmaps for n channels
1290-
got_multiple_cmaps = isinstance(render_params.cmap_params, list)
1291-
if got_multiple_cmaps:
1289+
# A list of cmap_params can be either user-supplied (one cmap per channel) or
1290+
# synthesized upstream to carry per-channel norms when the user only set `norm`
1291+
# (or `palette + norm=list`). The synthesized form must not trigger the
1292+
# blending warning or conflict with `palette`.
1293+
if isinstance(render_params.cmap_params, list):
1294+
got_multiple_cmaps = True
1295+
user_supplied_multi_cmaps = any(not cp.cmap_is_default for cp in render_params.cmap_params)
1296+
if len(render_params.cmap_params) != n_channels:
1297+
raise ValueError("If 'cmap' is provided, its length must match the number of channels.")
1298+
else:
1299+
got_multiple_cmaps = False
1300+
user_supplied_multi_cmaps = False
1301+
1302+
if user_supplied_multi_cmaps:
12921303
logger.warning(
12931304
"You're blending multiple cmaps. "
12941305
"If the plot doesn't look like you expect, it might be because your "
@@ -1297,10 +1308,6 @@ def _render_images(
12971308
"Consider using 'palette' instead."
12981309
)
12991310

1300-
# not using got_multiple_cmaps here because of ruff :(
1301-
if isinstance(render_params.cmap_params, list) and len(render_params.cmap_params) != n_channels:
1302-
raise ValueError("If 'cmap' is provided, its length must match the number of channels.")
1303-
13041311
# Detect RGB(A) images by channel names — skip when user overrides with palette/cmap
13051312
is_rgb, has_alpha = _is_rgb_image(channels)
13061313
has_explicit_cmap = (
@@ -1527,8 +1534,9 @@ def _render_images(
15271534
zorder=render_params.zorder,
15281535
)
15291536

1530-
# 2C) Image has n channels and palette info
1531-
elif palette is not None and not got_multiple_cmaps:
1537+
# 2C) palette set; also covers `palette + norm=list` since synthesized
1538+
# default cmaps don't conflict and per-channel norms are already in `layers`.
1539+
elif palette is not None and not user_supplied_multi_cmaps:
15321540
if len(palette) != n_channels:
15331541
raise ValueError("If 'palette' is provided, its length must match the number of channels.")
15341542

@@ -1568,7 +1576,7 @@ def _render_images(
15681576
)
15691577

15701578
# 2D) Image has n channels, no palette but cmap info
1571-
elif palette is not None and got_multiple_cmaps:
1579+
elif palette is not None and user_supplied_multi_cmaps:
15721580
raise ValueError("If 'palette' is provided, 'cmap' must be None.")
15731581

15741582
# Collect channel legend entries (single point for all multi-channel paths)

src/spatialdata_plot/pl/utils.py

Lines changed: 10 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -2989,15 +2989,22 @@ def _validate_image_render_params(
29892989
)
29902990
element_params[el]["palette"] = palette
29912991

2992+
expected_len = len(channel) if channel is not None else len(spatial_element_ch)
2993+
29922994
cmap = param_dict["cmap"]
29932995
if cmap is not None:
2994-
expected_len = len(channel) if channel is not None else len(spatial_element_ch)
29952996
if len(cmap) == 1:
29962997
cmap = cmap * expected_len
29972998
if len(cmap) != expected_len:
2998-
cmap = None
2999+
raise ValueError(
3000+
f"Length of 'cmap' list ({len(cmap)}) must match the number of channels ({expected_len})."
3001+
)
29993002
element_params[el]["cmap"] = cmap
3000-
element_params[el]["norm"] = param_dict["norm"]
3003+
3004+
norm = param_dict["norm"]
3005+
if isinstance(norm, list) and len(norm) > 1 and len(norm) != expected_len:
3006+
raise ValueError(f"Length of 'norm' list ({len(norm)}) must match the number of channels ({expected_len}).")
3007+
element_params[el]["norm"] = norm
30013008
scale = param_dict["scale"]
30023009
if scale and isinstance(param_dict["sdata"][el], DataTree):
30033010
if scale not in list(param_dict["sdata"][el].keys()) and scale != "full":

tests/pl/test_render_images.py

Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -496,6 +496,38 @@ def test_norm_list_without_explicit_cmap():
496496
plt.close(fig)
497497

498498

499+
# Regression tests for #622: misleading 'cmap' errors when norm/palette interact.
500+
def test_norm_list_wrong_length_raises_with_norm_message():
501+
# Without an explicit cmap the user only set norm; the error must mention norm,
502+
# not cmap, and report both lengths.
503+
sdata = _make_multichannel_sdata()
504+
with pytest.raises(ValueError, match=r"'norm' list \(2\).*channels \(3\)"):
505+
sdata.pl.render_images("img", norm=[Normalize(0, 1), Normalize(0, 2)]).pl.show()
506+
507+
508+
def test_cmap_wrong_length_with_norm_list_no_longer_silent():
509+
# Previously the wrong-length cmap was silently nulled when norm was a list of
510+
# the correct length, hiding the bug. It must now raise just like the no-norm path.
511+
sdata = _make_multichannel_sdata()
512+
with pytest.raises(ValueError, match=r"'cmap' list \(2\).*channels \(3\)"):
513+
sdata.pl.render_images("img", cmap=["Reds", "Greens"], norm=[Normalize(0, 1)] * 3).pl.show()
514+
515+
516+
def test_palette_with_norm_list_renders():
517+
# palette + per-channel norms used to fail with "If 'palette' is provided, 'cmap'
518+
# must be None." even though the user never passed cmap. Should now render.
519+
sdata = _make_multichannel_sdata()
520+
fig, ax = plt.subplots()
521+
sdata.pl.render_images("img", palette=["red", "green", "blue"], norm=[Normalize(0, 1)] * 3).pl.show(ax=ax)
522+
plt.close(fig)
523+
524+
525+
def test_palette_with_explicit_cmap_list_still_raises():
526+
sdata = _make_multichannel_sdata()
527+
with pytest.raises(ValueError, match="palette.*cmap"):
528+
sdata.pl.render_images("img", palette=["red", "green", "blue"], cmap=["Reds", "Greens", "Blues"]).pl.show()
529+
530+
499531
def test_cmap_matches_selected_channels_not_full_image(sdata_blobs: SpatialData):
500532
"""Cmap length should be validated against selected channels, not the full image channel count."""
501533
# blobs_image has 3 channels; select 1 with a matching length-1 cmap

0 commit comments

Comments
 (0)