Skip to content

Commit fbfa9b4

Browse files
committed
refactor(validate): decompose _type_check_params into per-parameter helpers (#716)
Split the 372-line, CCN~164 _type_check_params into 19 module-private _check_* helpers, each validating one parameter group in place; the function is now a flat ordered sequence of calls. Behavior-preserving: the call order (hence the first-raised error for any input) and all param_dict mutations are identical. Verified by a 370-case old-vs-new differential (0 mismatches) and the full non-visual suite (510 passed). Notable: contour_px keeps two checks at their original positions (type before color, range after outline) and _check_contour_px_range re-fetches contour_px from param_dict; the interleaved cmap/palette/groups region stays one helper. Adds a direct order-preservation test (color before contour_px range).
1 parent e454700 commit fbfa9b4

2 files changed

Lines changed: 75 additions & 1 deletion

File tree

src/spatialdata_plot/pl/_validate.py

Lines changed: 60 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -368,7 +368,7 @@ def _validate_col_for_column_table(
368368
return col_for_color, table_name
369369

370370

371-
def _type_check_params(param_dict: dict[str, Any], element_type: str) -> dict[str, Any]:
371+
def _check_colorbar(param_dict: dict[str, Any]) -> None:
372372
colorbar = param_dict.get("colorbar", "auto")
373373
if colorbar not in {True, False, None, "auto"}:
374374
raise TypeError("Parameter 'colorbar' must be one of True, False or 'auto'.")
@@ -377,6 +377,8 @@ def _type_check_params(param_dict: dict[str, Any], element_type: str) -> dict[st
377377
if colorbar_params is not None and not isinstance(colorbar_params, dict):
378378
raise TypeError("Parameter 'colorbar_params' must be a dictionary or None.")
379379

380+
381+
def _check_element(param_dict: dict[str, Any], element_type: str) -> None:
380382
element = param_dict.get("element")
381383
if element is not None and not isinstance(element, str):
382384
raise ValueError(
@@ -392,6 +394,8 @@ def _type_check_params(param_dict: dict[str, Any], element_type: str) -> dict[st
392394
elif element_type == "shapes":
393395
param_dict["element"] = [element] if element is not None else list(param_dict["sdata"].shapes.keys())
394396

397+
398+
def _check_channel(param_dict: dict[str, Any]) -> None:
395399
channel = param_dict.get("channel")
396400
if channel is not None and not isinstance(channel, list | str | int):
397401
raise TypeError("Parameter 'channel' must be a string, an integer, or a list of strings or integers.")
@@ -404,10 +408,14 @@ def _type_check_params(param_dict: dict[str, Any], element_type: str) -> dict[st
404408
elif "channel" in param_dict:
405409
param_dict["channel"] = [channel] if channel is not None else None
406410

411+
412+
def _check_contour_px_type(param_dict: dict[str, Any]) -> None:
407413
contour_px = param_dict.get("contour_px")
408414
if contour_px and not isinstance(contour_px, int):
409415
raise TypeError("Parameter 'contour_px' must be an integer.")
410416

417+
418+
def _check_color(param_dict: dict[str, Any], element_type: str) -> None:
411419
color = param_dict.get("color")
412420
if color and element_type in {
413421
"shapes",
@@ -440,6 +448,8 @@ def _type_check_params(param_dict: dict[str, Any], element_type: str) -> dict[st
440448
elif "color" in param_dict and element_type != "images":
441449
param_dict["col_for_color"] = None
442450

451+
452+
def _check_outline(param_dict: dict[str, Any], element_type: str) -> None:
443453
outline_width = param_dict.get("outline_width")
444454
if outline_width:
445455
# outline_width only exists for shapes at the moment
@@ -520,12 +530,17 @@ def _type_check_params(param_dict: dict[str, Any], element_type: str) -> dict[st
520530
else:
521531
param_dict["outline_color"] = Color(outline_color)
522532

533+
534+
def _check_contour_px_range(param_dict: dict[str, Any]) -> None:
535+
contour_px = param_dict.get("contour_px")
523536
if contour_px is not None and contour_px < 2:
524537
raise ValueError(
525538
"Parameter 'contour_px' must be >= 2; values below 2 produce no visible outline "
526539
"(a 1x1 erosion is the identity transformation)."
527540
)
528541

542+
543+
def _check_alpha(param_dict: dict[str, Any], element_type: str) -> None:
529544
alpha = param_dict.get("alpha")
530545
if alpha is not None:
531546
if not isinstance(alpha, float | int):
@@ -536,6 +551,8 @@ def _type_check_params(param_dict: dict[str, Any], element_type: str) -> dict[st
536551
# set default alpha for points if not given by user explicitly or implicitly (as part of color)
537552
param_dict["alpha"] = 1.0
538553

554+
555+
def _check_fill_alpha(param_dict: dict[str, Any], element_type: str) -> None:
539556
fill_alpha = param_dict.get("fill_alpha")
540557
if fill_alpha is not None:
541558
if not isinstance(fill_alpha, float | int):
@@ -549,6 +566,8 @@ def _type_check_params(param_dict: dict[str, Any], element_type: str) -> dict[st
549566
# set default fill_alpha for labels if not given by user explicitly or implicitly (as part of color)
550567
param_dict["fill_alpha"] = 0.4
551568

569+
570+
def _check_cmap_palette_groups(param_dict: dict[str, Any], element_type: str) -> None:
552571
cmap = param_dict.get("cmap")
553572
palette = param_dict.get("palette")
554573
if cmap is not None and palette is not None:
@@ -597,10 +616,14 @@ def _type_check_params(param_dict: dict[str, Any], element_type: str) -> dict[st
597616
else:
598617
raise TypeError("Parameter 'cmap' must be a string, a Colormap, or a list of these types.")
599618

619+
620+
def _check_na_color(param_dict: dict[str, Any]) -> None:
600621
# validation happens within Color constructor (images don't use na_color)
601622
if "na_color" in param_dict:
602623
param_dict["na_color"] = Color(param_dict.get("na_color"))
603624

625+
626+
def _check_norm(param_dict: dict[str, Any], element_type: str) -> None:
604627
norm = param_dict.get("norm")
605628
if norm is not None:
606629
if element_type == "images":
@@ -618,6 +641,8 @@ def _type_check_params(param_dict: dict[str, Any], element_type: str) -> dict[st
618641
if element_type == "graph" and not isinstance(norm, Normalize):
619642
raise TypeError("Parameter 'norm' must be a Normalize instance.")
620643

644+
645+
def _check_scale(param_dict: dict[str, Any], element_type: str) -> None:
621646
scale = param_dict.get("scale")
622647
if scale is not None:
623648
if element_type in {"images", "labels"} and not isinstance(scale, str):
@@ -628,13 +653,17 @@ def _type_check_params(param_dict: dict[str, Any], element_type: str) -> dict[st
628653
if scale < 0:
629654
raise ValueError("Parameter 'scale' must be a positive number.")
630655

656+
657+
def _check_size(param_dict: dict[str, Any]) -> None:
631658
size = param_dict.get("size")
632659
if size:
633660
if not isinstance(size, float | int):
634661
raise TypeError("Parameter 'size' must be numeric.")
635662
if size < 0:
636663
raise ValueError("Parameter 'size' must be a positive number.")
637664

665+
666+
def _check_shape(param_dict: dict[str, Any], element_type: str) -> None:
638667
shape = param_dict.get("shape")
639668
if element_type == "shapes" and shape is not None:
640669
valid_shapes = {"circle", "hex", "visium_hex", "square"}
@@ -643,6 +672,8 @@ def _type_check_params(param_dict: dict[str, Any], element_type: str) -> dict[st
643672
if shape not in valid_shapes:
644673
raise ValueError(f"'{shape}' is not supported for 'shape', please choose from {valid_shapes}.")
645674

675+
676+
def _check_table(param_dict: dict[str, Any]) -> None:
646677
table_name = param_dict.get("table_name")
647678
table_layer = param_dict.get("table_layer")
648679
if table_name and not isinstance(param_dict["table_name"], str):
@@ -690,10 +721,14 @@ def _ensure_table_and_layer_exist_in_sdata(
690721

691722
_ensure_table_and_layer_exist_in_sdata(param_dict.get("sdata"), table_name, table_layer)
692723

724+
725+
def _check_method(param_dict: dict[str, Any]) -> None:
693726
method = param_dict.get("method")
694727
if method not in ["matplotlib", "datashader", None]:
695728
raise ValueError("If specified, parameter 'method' must be either 'matplotlib' or 'datashader'.")
696729

730+
731+
def _check_ds_reduction(param_dict: dict[str, Any]) -> None:
697732
valid_ds_reduction_methods = [
698733
"sum",
699734
"mean",
@@ -710,6 +745,8 @@ def _ensure_table_and_layer_exist_in_sdata(
710745
if ds_reduction and (ds_reduction not in valid_ds_reduction_methods):
711746
raise ValueError(f"Parameter 'ds_reduction' must be one of the following: {valid_ds_reduction_methods}.")
712747

748+
749+
def _check_graph_params(param_dict: dict[str, Any], element_type: str) -> None:
713750
if element_type == "graph":
714751
for key in ("connectivity_key",):
715752
val = param_dict.get(key)
@@ -739,6 +776,28 @@ def _ensure_table_and_layer_exist_in_sdata(
739776
if val is not None and not isinstance(val, bool):
740777
raise TypeError(f"Parameter '{key}' must be a boolean.")
741778

779+
780+
def _type_check_params(param_dict: dict[str, Any], element_type: str) -> dict[str, Any]:
781+
# Call order is the contract: the first-raised error and in-place mutations must match the pre-split form.
782+
_check_colorbar(param_dict)
783+
_check_element(param_dict, element_type)
784+
_check_channel(param_dict)
785+
_check_contour_px_type(param_dict)
786+
_check_color(param_dict, element_type)
787+
_check_outline(param_dict, element_type)
788+
_check_contour_px_range(param_dict) # must stay after outline (preserves first-error order)
789+
_check_alpha(param_dict, element_type)
790+
_check_fill_alpha(param_dict, element_type)
791+
_check_cmap_palette_groups(param_dict, element_type)
792+
_check_na_color(param_dict)
793+
_check_norm(param_dict, element_type)
794+
_check_scale(param_dict, element_type)
795+
_check_size(param_dict)
796+
_check_shape(param_dict, element_type)
797+
_check_table(param_dict)
798+
_check_method(param_dict)
799+
_check_ds_reduction(param_dict)
800+
_check_graph_params(param_dict, element_type)
742801
return param_dict
743802

744803

tests/pl/test_utils.py

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -273,6 +273,21 @@ def test_extract_scalar_value():
273273
assert _extract_scalar_value([], default=1.0) == 1.0
274274

275275

276+
def test_type_check_params_preserves_validation_order():
277+
"""#716: decomposition must preserve call order, so the first error for a multiply-invalid input is
278+
unchanged -- `color` is validated before the `contour_px < 2` range check.
279+
"""
280+
from spatialdata_plot.pl._validate import _type_check_params
281+
282+
# color=5 raises in the color block, before the contour_px range check -> color error wins.
283+
with pytest.raises(TypeError, match="Parameter 'color' must be a string or a tuple/list of floats."):
284+
_type_check_params({"element": "x", "color": 5, "contour_px": 1, "sdata": None}, "labels")
285+
286+
# a valid RGB tuple (no sdata-dependent collision check) lets the same input reach contour_px.
287+
with pytest.raises(ValueError, match="Parameter 'contour_px' must be >= 2"):
288+
_type_check_params({"element": "x", "color": (1.0, 0.0, 0.0), "contour_px": 1, "sdata": None}, "labels")
289+
290+
276291
def test_plot_can_handle_rgba_color_specifications(sdata_blobs: SpatialData):
277292
"""Test handling of RGBA color specifications."""
278293
# Test with RGBA tuple

0 commit comments

Comments
 (0)