Maintainability quick wins: dedup, redundant copies, O(K^2) lookup#701
Merged
Conversation
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]`).
58755b9 to
4a16a34
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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
ColorLiketype alias. It was defined three times (basic.py,utils.py,render_params.py), and therender_params.pycopy had silently droppedlist[float]— so the alias meant different things in different modules.render_params.py(the lowest module) is now canonical;basic.py/utils.pyimport it._map_color_seg:map_array(seg.copy(), …)→map_array(seg, …)(×6). skimagemap_arraynever 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-channelimg.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-categorylist.index/inscan (O(K²) for K categories) with a single{category: first_index}dict (O(K)). Bites on large category counts._get_subplots— no callers insrc/(only a test exercised it) — and its test.Verification
multi-channel image(cmap list + single cmap) andlabels(categorical + continuous) renders are byte-identical tomain(RGBA buffers compared vianp.array_equal).pytest -k "not test_plot").test_plot_*baselines run on CI; none should change (output is byte-identical).Out of scope (flagged during the audit, deliberately not included)
spatialdataalready hard-depsdatashader/dask/geopandas(so the supply-chain angle is moot), the lazy import touches 36ds.sites across 3 modules, and hard-dep-vs-optional is a real design call. Better as its own PR.colorbar_paramsTypedDict —_split_colorbar_paramspasses unknown keys through to matplotlib, so a closedTypedDictwould wrongly reject valid passthrough kwargs; needs an API-typing decision.