Skip to content

Commit 837c258

Browse files
timtreisclaude
andcommitted
Address review findings on alignment skeleton
- Replace private `sdata._gen_elements()` with public `gen_elements()` - Replace dict-style `sdata[key]` lookup with explicit element-type search - Add subprocess timeout (30s) to lazy-import hygiene test - Document shallow X sharing in `materialise_obs` docstring - Document JAX array retention in stalign metadata comment - Document camelCase convention in STalignRegistrationConfig docstring - Broaden landmark type hints to accept Sequence and np.ndarray - Remove stale TODO comment from _stalign_helpers.py Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
1 parent 42d74ab commit 837c258

6 files changed

Lines changed: 39 additions & 13 deletions

File tree

src/squidpy/experimental/tl/_align/_api.py

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -8,8 +8,11 @@
88

99
from __future__ import annotations
1010

11+
from collections.abc import Sequence
1112
from typing import TYPE_CHECKING, Any, Literal
1213

14+
import numpy as np
15+
1316
from squidpy.experimental.tl._align._backends import get_backend
1417
from squidpy.experimental.tl._align._io import (
1518
apply_affine_to_cs,
@@ -160,8 +163,8 @@ def align_by_landmarks(
160163
cs_name_query: str | None = None,
161164
scale_ref: str | None = None,
162165
scale_query: str | None = None,
163-
landmarks_ref: tuple[tuple[float, float], ...] | None = None,
164-
landmarks_query: tuple[tuple[float, float], ...] | None = None,
166+
landmarks_ref: Sequence[tuple[float, float]] | np.ndarray | None = None,
167+
landmarks_query: Sequence[tuple[float, float]] | np.ndarray | None = None,
165168
*,
166169
model: Literal["similarity", "affine"] = "similarity",
167170
output_mode: Literal["affine", "return"] = "affine",

src/squidpy/experimental/tl/_align/_backends/_stalign.py

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -82,9 +82,11 @@ def align_obs(
8282
),
8383
metadata={
8484
"flavour": "stalign",
85-
# Escape hatch for power users who want the diffeomorphic
86-
# part (velocity field, velocity grid, affine init) rather
87-
# than just the materialised displacement.
85+
# Escape hatch: the full STalignResult (velocity field,
86+
# velocity grid, affine init) for power users who need
87+
# the diffeomorphic map. This keeps the JAX arrays alive
88+
# in memory -- callers who only need the displacement
89+
# should drop this key or use ``output_mode='obs'``.
8890
"stalign_result": stalign_result,
8991
},
9092
)

src/squidpy/experimental/tl/_align/_backends/_stalign_helpers.py

Lines changed: 0 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -56,10 +56,6 @@ def extract_landmarks(adata: AnnData, key: str) -> np.ndarray:
5656
raise KeyError(f"Key `{key}` not found in `adata.obsm` or `adata.uns`.")
5757

5858

59-
# TODO: are these duplicated? I would imagine its
60-
# better to keep image transform functions under some place
61-
62-
6359
def to_row_col(points: np.ndarray, *, point_order: PointOrder) -> np.ndarray:
6460
"""Convert coordinates to row-column order."""
6561
arr = _validate_points(points, name="points")

src/squidpy/experimental/tl/_align/_backends/_stalign_tools.py

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -54,6 +54,13 @@ class STalignPreprocessConfig:
5454

5555
@dataclass(slots=True)
5656
class STalignRegistrationConfig:
57+
"""LDDMM registration hyperparameters.
58+
59+
Field names (``sigmaM``, ``epL``, etc.) preserve the conventions from
60+
the STalign paper and reference implementation to keep them
61+
recognisable when cross-referencing the literature.
62+
"""
63+
5764
a: float = 500.0
5865
p: float = 2.0
5966
expand: float = 2.0

src/squidpy/experimental/tl/_align/_io.py

Lines changed: 21 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -213,7 +213,7 @@ def apply_affine_to_cs(
213213

214214
if pair.query_container is not None and pair.query_element_key is not None:
215215
sdata = pair.query_container if inplace else _shallow_copy_sdata(pair.query_container)
216-
element = sdata[pair.query_element_key]
216+
element = _get_element(sdata, pair.query_element_key)
217217
set_transformation(element, affine.to_spatialdata(), to_coordinate_system=target_cs)
218218
return None if inplace else sdata
219219

@@ -222,7 +222,9 @@ def apply_affine_to_cs(
222222
moving_cs = pair.query_cs
223223
sd_affine = affine.to_spatialdata()
224224
touched_any = False
225-
for _etype, _name, element in sdata._gen_elements(include_tables=False):
225+
for _etype, _name, element in sdata.gen_elements():
226+
if isinstance(element, AnnData):
227+
continue
226228
element_transforms = get_transformation(element, get_all=True)
227229
if moving_cs not in element_transforms:
228230
continue
@@ -258,6 +260,13 @@ def materialise_obs(
258260
add the deltas. When the source query lives inside a SpatialData, the new
259261
AnnData is registered as ``sdata.tables[key_added]``; otherwise it is
260262
returned directly.
263+
264+
.. note::
265+
266+
The returned AnnData **shares** ``X`` and ``var`` with the source
267+
query by reference to avoid copying potentially-large expression
268+
matrices. Mutating one will affect the other. Call
269+
``.copy()`` on the result if you need full independence.
261270
"""
262271
if not isinstance(pair.query, AnnData):
263272
raise TypeError("materialise_obs only works for `align_obs`; `pair.query` must be an AnnData.")
@@ -297,13 +306,21 @@ def materialise_obs(
297306
return new_adata
298307

299308

309+
def _get_element(sdata: SpatialData, key: str) -> object:
310+
"""Look up a spatial element by name across all element types."""
311+
for attr in ("images", "labels", "points", "shapes", "tables"):
312+
store = getattr(sdata, attr)
313+
if key in store:
314+
return store[key]
315+
raise KeyError(f"Element {key!r} not found in the SpatialData object.")
316+
317+
300318
def _shallow_copy_sdata(sdata: SpatialData) -> SpatialData:
301319
"""Shallow copy of a SpatialData object for ``inplace=False`` writeback paths.
302320
303321
Uses :meth:`SpatialData.subset` over every element so tables and
304322
``attrs`` propagate the same way spatialdata's own subsetting handles
305323
them, rather than reconstructing via the ``__init__`` constructor.
306324
"""
307-
element_names = [name for _, name, _ in sdata._gen_elements(include_tables=True)]
325+
element_names = [name for _, name, _ in sdata.gen_elements()]
308326
return sdata.subset(element_names, filter_tables=False, include_orphan_tables=True)
309-

tests/experimental/tl/test_align_skeleton.py

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -132,6 +132,7 @@ def test_optional_deps_not_imported_at_import_time() -> None:
132132
),
133133
],
134134
text=True,
135+
timeout=30,
135136
).strip()
136137
assert out == "", f"Optional deps imported by `import squidpy`: {out}"
137138

0 commit comments

Comments
 (0)