Skip to content

Commit 79b99fa

Browse files
timtreisclaude
andcommitted
Address adversarial review findings for per-channel norm
1. Improve error message when norm list + cmap resolution fails — now mentions cmap/channel mismatch as possible cause instead of the misleading "multiple colormaps are used" 2. Unwrap length-1 cmap_params lists to scalar so single-channel images still take the fast path (proper norm in imshow + colorbar) 3. Make Normalize copy unconditional in the multi-channel compositing loop — eliminates fragile conditional that only copied auto-ranging norms, preventing any future cross-channel mutation bugs Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
1 parent 9ba4c48 commit 79b99fa

3 files changed

Lines changed: 10 additions & 5 deletions

File tree

src/spatialdata_plot/pl/basic.py

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -658,15 +658,20 @@ def render_images(
658658
else:
659659
if isinstance(norm, list):
660660
raise ValueError(
661-
"Parameter 'norm' can only be a list when multiple colormaps are used. "
662-
"Pass a list of colormaps via 'cmap' or use a single Normalize."
661+
"Parameter 'norm' can only be a list when a matching list of colormaps is resolved. "
662+
"Pass an explicit list of colormaps via 'cmap' whose length matches the number of "
663+
"channels, or use a single Normalize."
663664
)
664665
cmap_params = _prepare_cmap_norm(
665666
cmap=cmap,
666667
norm=norm,
667668
na_color=param_values["na_color"],
668669
**kwargs,
669670
)
671+
# Unwrap length-1 list so the single-channel rendering path
672+
# (which checks `not isinstance(cmap_params, list)`) still works.
673+
if isinstance(cmap_params, list) and len(cmap_params) == 1:
674+
cmap_params = cmap_params[0]
670675
sdata.plotting_tree[f"{n_steps + 1}_render_images"] = ImageRenderParams(
671676
element=element,
672677
channel=param_values["channel"],

src/spatialdata_plot/pl/render.py

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1313,8 +1313,8 @@ def _render_images(
13131313
else:
13141314
ch_norm = render_params.cmap_params.norm
13151315

1316-
# Auto-ranging norms are stateful — copy so each channel normalizes independently
1317-
if isinstance(ch_norm, Normalize) and (ch_norm.vmin is None or ch_norm.vmax is None):
1316+
# Normalize objects are stateful — always copy to prevent cross-channel mutation
1317+
if isinstance(ch_norm, Normalize):
13181318
ch_norm = copy(ch_norm)
13191319

13201320
layers[ch] = ch_norm(layers[ch])

tests/pl/test_render_images.py

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -478,5 +478,5 @@ def test_norm_list_with_invalid_element_raises():
478478
def test_norm_list_without_cmap_list_raises():
479479
"""Norm list requires explicit cmap list."""
480480
sdata = _make_multichannel_sdata()
481-
with pytest.raises(ValueError, match="can only be a list when multiple colormaps"):
481+
with pytest.raises(ValueError, match="can only be a list when a matching list of colormaps"):
482482
sdata.pl.render_images("img", norm=[Normalize(0, 1)] * 3).pl.show()

0 commit comments

Comments
 (0)