Skip to content

Maintainability quick wins: dedup, redundant copies, O(K^2) lookup#701

Merged
timtreis merged 2 commits into
mainfrom
refactor/maintainability-quick-wins
Jun 6, 2026
Merged

Maintainability quick wins: dedup, redundant copies, O(K^2) lookup#701
timtreis merged 2 commits into
mainfrom
refactor/maintainability-quick-wins

Conversation

@timtreis
Copy link
Copy Markdown
Member

@timtreis timtreis commented Jun 6, 2026

Summary

A small, low-risk maintainability sweep from a refactor audit of main. No public API or behavioral change. The bugs and larger refactors from the same audit are tracked separately in #694#700.

Changes

  • Single-source the ColorLike type alias. It was defined three times (basic.py, utils.py, render_params.py), and the render_params.py copy had silently dropped list[float] — so the alias meant different things in different modules. render_params.py (the lowest module) is now canonical; basic.py/utils.py import it.
  • Drop provably-dead copies:
    • _map_color_seg: map_array(seg.copy(), …)map_array(seg, …) (×6). skimage map_array never mutates its input (verified), so the full-label-image copy was pure waste — and it runs twice per labels render when outlines are on.
    • _render_images: per-channel img.sel(c=ch).copy(deep=True).squeeze().squeeze(). The entry is only read (min/max) and then replaced by a fresh array (np.full/ch_norm(…)), so the deep copy was never mutated.
    • _render_points: …flatten().copy()…flatten(). numpy/matrix .flatten() already returns a fresh array, so the trailing .copy() double-allocated.
  • _extract_colors_from_table_uns: replace the per-category list.index/in scan (O(K²) for K categories) with a single {category: first_index} dict (O(K)). Bites on large category counts.
  • Dedup the "blending multiple cmaps" warning into a module constant (the two copies had already drifted by a sentence).
  • Remove _get_subplots — no callers in src/ (only a test exercised it) — and its test.

Verification

  • multi-channel image (cmap list + single cmap) and labels (categorical + continuous) renders are byte-identical to main (RGBA buffers compared via np.array_equal).
  • Non-visual test suite: 356 passed, 1 skipped (pytest -k "not test_plot").
  • pre-commit: ruff check, ruff format, mypy all pass.
  • Visual test_plot_* baselines run on CI; none should change (output is byte-identical).

Out of scope (flagged during the audit, deliberately not included)

  • datashader import hygiene / lazy importspatialdata already hard-deps datashader/dask/geopandas (so the supply-chain angle is moot), the lazy import touches 36 ds. sites across 3 modules, and hard-dep-vs-optional is a real design call. Better as its own PR.
  • colorbar_params TypedDict_split_colorbar_params passes unknown keys through to matplotlib, so a closed TypedDict would wrongly reject valid passthrough kwargs; needs an API-typing decision.

timtreis added 2 commits June 7, 2026 01:18
A small low-risk sweep from a maintainability audit of main. No public API or
behavioral change; multi-channel image and labels renders verified byte-identical
to main (RGBA buffers compared).

- Single-source the `ColorLike` type alias. It was defined three times, and the
  copy in render_params.py had silently dropped `list[float]`. render_params.py
  is now the canonical definition; basic.py and utils.py import it.
- Drop provably-dead copies:
  - `_map_color_seg`: `map_array(seg.copy(), ...)` -> `map_array(seg, ...)` (x6).
    skimage `map_array` never mutates its input.
  - `_render_images`: per-channel `img.sel(c=ch).copy(deep=True)` -> `.squeeze()`.
    The entry is only read (min/max) then replaced by a fresh array.
  - `_render_points`: `...flatten().copy()` -> `...flatten()`. `flatten()` already
    returns a fresh array.
- `_extract_colors_from_table_uns`: replace the per-category `list.index`/`in`
  scan (O(K^2) for K categories) with a single `{category: first_index}` dict.
- Hoist the duplicated "blending multiple cmaps" warning into a module constant
  (the two copies had already drifted by a sentence).
- Remove `_get_subplots` (no callers in src; only a test exercised it) and its test.
Follow-up cleanup (no behavioral change):
- `_extract_colors_from_table_uns`: the category->index map is built from
  pandas `.categories` (always unique), so a dict comprehension replaces the
  `setdefault` loop without changing first-occurrence semantics.
- tests/pl/test_utils.py: import the canonical `ColorLike` from render_params
  instead of keeping a local copy that had drifted to the old narrow form
  (missing `list[float]`).
@timtreis timtreis force-pushed the refactor/maintainability-quick-wins branch from 58755b9 to 4a16a34 Compare June 6, 2026 23:21
@timtreis timtreis merged commit 78d9233 into main Jun 6, 2026
5 of 8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant