Skip to content

Commit 21b5477

Browse files
committed
refactor(render_shapes): use Path.circle, lean comments (review)
- circle path: use mpath.Path.circle((x,y), r) instead of hand-rolling unit_circle.vertices*r+center (byte-identical; drops the hoist + comment). - trim narrating/duplicated comments (build docstring, reuse comment) and correct the trans-once comment (the old per-collection transform was redundant work, not a visible double-transform — verified main-vs-branch byte-identical).
1 parent cf14b3e commit 21b5477

2 files changed

Lines changed: 8 additions & 17 deletions

File tree

src/spatialdata_plot/pl/_geometry.py

Lines changed: 5 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -103,10 +103,9 @@ def _build_shape_paths(
103103
) -> tuple[list[mpath.Path], list[int], int]:
104104
"""Build matplotlib ``Path``s from shape geometries, once.
105105
106-
Geometry is independent of colour/alpha, so it is built a single time and shared across the fill and
107-
outline ``PathCollection``s in :func:`_render_shapes`. Using ``Path`` objects (the form a
108-
``PatchCollection`` bakes internally anyway) and a ``PathCollection`` avoids constructing one
109-
``matplotlib.patches.*`` object per shape — the dominant cost for large shape elements.
106+
Built once and shared across the fill and outline ``PathCollection``s in :func:`_render_shapes`.
107+
Emitting ``Path``s directly avoids constructing one ``matplotlib.patches.*`` object per shape — the
108+
dominant cost for large shape elements.
110109
111110
Returns
112111
-------
@@ -135,7 +134,6 @@ def _build_shape_paths(
135134

136135
# Resolve the scale scalar once instead of per shape.
137136
scale_value = _extract_scalar_value(scale, default=1.0)
138-
unit_circle = mpath.Path.unit_circle()
139137

140138
paths: list[mpath.Path] = []
141139
row_idx: list[int] = []
@@ -154,10 +152,7 @@ def _build_shape_paths(
154152
row_idx.append(i)
155153
elif geom_type == "Point":
156154
radius_value = _extract_scalar_value(radii[i], default=0.0) if radii is not None else 0.0
157-
# unit circle scaled by radius and translated to the centre — identical to the path a
158-
# PatchCollection bakes from mpatches.Circle((x, y), radius).
159-
verts = unit_circle.vertices * (radius_value * scale_value) + (geom.x, geom.y)
160-
paths.append(mpath.Path(verts, unit_circle.codes))
155+
paths.append(mpath.Path.circle((geom.x, geom.y), radius_value * scale_value))
161156
row_idx.append(i)
162157

163158
return paths, row_idx, len(geoms)
@@ -209,9 +204,7 @@ def _get_collection_shape(
209204
else:
210205
outline_c = [None] * fill_c.shape[0]
211206

212-
# Build (or reuse) the matplotlib paths. Geometry is colour-independent, so the
213-
# caller can build it once via `_build_shape_paths` and share it across the fill
214-
# and outline collections instead of rebuilding it on every call.
207+
# Reuse the shared paths when provided (see _build_shape_paths), else build them.
215208
paths, row_idx, n_shapes = prebuilt_paths if prebuilt_paths is not None else _build_shape_paths(shapes, s)
216209

217210
if not paths:

src/spatialdata_plot/pl/render.py

Lines changed: 3 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -922,12 +922,10 @@ def _draw_centroids(xy: np.ndarray, radius: float | None = None) -> None:
922922
cax = _build_ds_colorbar(reduction_bounds, norm, render_params.cmap_params.cmap)
923923

924924
elif method == "matplotlib":
925-
# Build the matplotlib paths once and share them across the fill and outline collections;
926-
# the geometry is identical, only colours/alpha/linewidth differ.
925+
# Build the paths once and share them across the fill and outline collections (geometry is
926+
# identical; only colours/alpha/linewidth differ), then apply the coordinate-system affine
927+
# once to the shared Path objects rather than once per collection.
927928
prebuilt_paths = _build_shape_paths(shapes, render_params.scale)
928-
# Apply the coordinate-system affine ONCE to the shared paths. The fill and outline
929-
# collections reference the same Path objects, so transforming per-collection (as the old
930-
# PatchCollection path did) applied `trans` 2-3x — a latent double-transform for polygons.
931929
for path in prebuilt_paths[0]:
932930
path.vertices = trans.transform(path.vertices)
933931

0 commit comments

Comments
 (0)