spatial_neigbours extensibility features and clarification by selmanozleyen · Pull Request #1147 · scverse/squidpy

selmanozleyen · 2026-04-02T11:16:36Z

fixes: #1102 and #1047

It's backward compatible and I am curious what the community might bring to this!

I clarified the docs on argument precedence
Added extensibility features
Added tests to ensure the function behaves as written in the docs
No breaking changes

for more information, see https://pre-commit.ci

…leyen/squidpy into feat/spatial_neighbours

for more information, see https://pre-commit.ci

…leyen/squidpy into feat/spatial_neighbours

codecov · 2026-04-02T12:06:15Z

Codecov Report

❌ Patch coverage is 87.46518% with 45 lines in your changes missing coverage. Please review.
✅ Project coverage is 73.82%. Comparing base (5b3c95d) to head (ea3f3ca).

Files with missing lines	Patch %	Lines
src/squidpy/gr/_build.py	80.41%	24 Missing and 4 partials ⚠️
src/squidpy/gr/neighbors.py	92.01%	11 Missing and 6 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1147      +/-   ##
==========================================
+ Coverage   73.56%   73.82%   +0.26%     
==========================================
  Files          44       45       +1     
  Lines        6929     7098     +169     
  Branches     1174     1178       +4     
==========================================
+ Hits         5097     5240     +143     
- Misses       1347     1369      +22     
- Partials      485      489       +4

Files with missing lines	Coverage Δ
src/squidpy/_docs.py	`94.44% <100.00%> (+0.24%)`	⬆️
src/squidpy/gr/neighbors.py	`92.01% <92.01%> (ø)`
src/squidpy/gr/_build.py	`81.21% <80.41%> (-7.31%)`	⬇️

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

…leyen/squidpy into feat/spatial_neighbours

for more information, see https://pre-commit.ci

…leyen/squidpy into feat/spatial_neighbours

grst

Thanks @selmanozleyen, that goes in the right direction. I put up some design questions up for discussion!

for more information, see https://pre-commit.ci

timtreis · 2026-04-11T16:59:08Z


+    Notes
+    -----
+    ``spatial_neighbors`` has 4 graph-construction modes:


Do we have a good explanation of these somewhere?

timtreis · 2026-04-11T16:59:49Z

+        "`spatial_neighbors_delaunay`, `spatial_neighbors_grid`, or "
+        "`spatial_neighbors_from_builder` instead.",
+        FutureWarning,
+        stacklevel=2,


I think your other future warning was stacklevel=3, how do you decide this?

stacklevel 2: user code -> spatial_neighbors() -> warnings.warn(...)

vs

stacklevel 3: user code -> spatial_neighbors() -> _resolve_graph_builder() -> warnings.warn(...)

Do we have this written down somewhere? I haven't really thought about this yet so I wonder if we should add that to some "how to Squidpy" doc somewhere?

timtreis · 2026-04-11T17:00:11Z

-    }
+@d.dedent
+def spatial_neighbors_from_builder(
+    adata: AnnData | SpatialData,


see other comment

timtreis · 2026-04-11T17:01:00Z

+
+
+def _prepare_spatial_neighbors_input(
+    adata: AnnData | SpatialData,


see other comment

timtreis · 2026-04-11T17:01:21Z

+def _prepare_spatial_neighbors_input(
+    adata: AnnData | SpatialData,
+    *,
+    spatial_key: str,


Align annotations across functions

timtreis · 2026-04-11T17:01:57Z

-    Return D^{-1/2} * A * D^{-1/2}, where D = diag(degrees(A)) and A = adj.
+@d.dedent
+def spatial_neighbors_grid(
+    adata: AnnData | SpatialData,


see other comment

timtreis · 2026-04-11T17:03:10Z

+
+    def __init__(
+        self,
+        radius: float | tuple[float, float] | None = None,


is this used?

yes by _filter_by_radius_interval. If I did this from scratch maybe I'd make it more complicated by having this filtering as a separate transform but this feels more appropiate for old implementation

timtreis · 2026-04-11T17:05:32Z

@@ -0,0 +1,440 @@
+"""Graph construction strategies for spatial neighbor graphs.


Quite some duplicates code, can you use a factory or prototcol instead?

Not sure if I agree. What exactly do you think needs deduplication here?

The two typevars here aren't modified anywhere. It could just be an ABC.

apply_percentile is the only function using the coord_type. Since at the point of _resolve_graph_builder we decide on the downstream path, we don't need that guard. With that guard removed, the coord_type becomes superflous and would further simplify the class.

Both apply_filters are identical, could be deduplicated by moving it into build of the ABC.

KNNBuilder.build_graph and RadiusBuilder.build_graph share quite some overlap in building their nn-graphs. Just the delauny stuff would need to be routed differently.

but I guess the biggest win would be removing the non-CSR build option? Is this sth we'll realistically ever encounter?

The factory/protocol comment doesn't make much sense in hind-sight after having gone through it in detail - still, quite some opportunity to remove LOCs

but I guess the biggest win would be removing the non-CSR build option?

I'd like to also give possibility to support cupy sparse or whatever sparse types jax might have for example

But I noticed I use scipy operations also in the builder path so let me just have a second look.

for more information, see https://pre-commit.ci

* [pre-commit.ci] pre-commit autoupdate updates: - [github.com/biomejs/pre-commit: v2.4.9 → v2.4.10](biomejs/pre-commit@v2.4.9...v2.4.10) - [github.com/tox-dev/pyproject-fmt: v2.20.0 → v2.21.0](tox-dev/pyproject-fmt@v2.20.0...v2.21.0) - [github.com/astral-sh/ruff-pre-commit: v0.15.8 → v0.15.9](astral-sh/ruff-pre-commit@v0.15.8...v0.15.9) * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci --------- Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>

for more information, see https://pre-commit.ci

shashkat

It looks amazing to me now! Just a few more minor suggestions. I really liked the idea of using specialized Postprocessor classes!

shashkat · 2026-04-20T01:06:55Z

    n_neighs
-        Number of neighboring tiles. Defaults to ``6``.
+        Number of neighboring tiles used to form the base grid connectivity.
+        Defaults to ``6``. On a Visium-like hexagonal grid, ``6`` corresponds to
+        the immediate surrounding spots, while smaller values such as ``3`` make
+        the first-ring graph deliberately sparser.


Maybe it would be useful to mention that there is no guarantee of what subset of neighbors would be chosen from a given ring, if n_neighs is less than the actual number of neighbors of a cell in the grid.
Just like this snippet from the latest-commit documentation of spatial_neighbors function:

- values such as ``n_neighs=4`` and ``n_neighs=6`` are the intended square-grid and hex-grid choices, respectively; - other values are accepted for backward compatibility, but their geometric interpretation is not guaranteed to match a continuous ring on the grid; - no clockwise or other within-ring ordering is part of the public API.

shashkat · 2026-04-20T01:13:35Z

+    def postprocessors(self) -> Sequence[GraphPostprocessor[GraphMatrixT]]:
+        """Return post-build processing steps for ``(adj, dst)``."""
+        return self._postprocessors


I am not sure if this method is offering any functionality.
We can possibly call the attribute _postprocessors as postprocessors and replace line 77 from for postprocessor in self.postprocessors(): to for postprocessor in self.postprocessors:.
What I understand is that this method can allow one to reorder or filter out the entities in _postprocessors attribute. But that can anyways be done when the postprocessors entities are created in the init method of a subclass of GraphBuilder.

shashkat · 2026-04-20T01:15:13Z

    def __init__(
        self,
        transform: str | Transform | None = None,
        set_diag: bool = False,
        percentile: float | None = None,
+        postprocessors: Sequence[GraphPostprocessor[GraphMatrixT]] = (),
    ) -> None:
        self.transform = Transform.NONE if transform is None else Transform(transform)
        self.set_diag = set_diag
        self.percentile = percentile
+        self._postprocessors: list[GraphPostprocessor[GraphMatrixT]] = list(postprocessors)


Is the transform argument used here? I think the transformation requirements would be encoded in postprocessors anyways, so we can remove it.

shashkat · 2026-04-20T01:20:44Z

+        if isinstance(radius, tuple):
+            postprocessors.append(DistanceIntervalPostprocessor(tuple(sorted(radius))))


If someone passes a single float or int to radius, DelaunayBuilder doesn't do anything with it I believe. Only if radius is a tuple[float, float], does it add the filter. I think we can fix this by doing something like the following.
change this:

if isinstance(radius, tuple): postprocessors.append(DistanceIntervalPostprocessor(tuple(sorted(radius))))

to this:

if isinstance(radius, (int, float)): radius = (0, radius) postprocessors.append(DistanceIntervalPostprocessor(tuple(sorted(radius))))

Then, we can also change the type suggestion for radius in spatial_neighbors_delaunay to float | tuple[float, float].

grst · 2026-04-22T06:02:02Z

+    def __init__(
+        self,
+        radius: float | tuple[float, float],
+        transform: str | Transform | None = None,


Adding these arguments here adds again some complexity/branching to the RadiusBuilder class, which makes it harder to reason about it and implement it on other backends.

An alternative would be to support these arguments only in the high-level function and instead support chaining of Builders and Postprocessors. Preprocessors would inherit the API from Builders and could be instantiated with a Builder or another already initialized preprocessor. E.g.

transform_interval_builder: Builder = TransformPreprocessor( DistanceIntervalPostprocessor(RadiusBuilder(...)) )

or

transform_interval_builder: Builder = RadiusBuilder(...).chain( DistanceIntervalPostprocessor ).chain( TransformPreprocessor )

I don't have a super strong opinion on this, just as an additional idea.

selmanozleyen and others added 9 commits April 2, 2026 13:06

init

5c08081

doc clarifications

5847d84

don't expose the builders

179ba06

[pre-commit.ci] auto fixes from pre-commit.com hooks

e187aba

for more information, see https://pre-commit.ci

deduplicate

cdb5699

Merge branch 'feat/spatial_neighbours' of https://github.com/selmanoz…

847a46b

…leyen/squidpy into feat/spatial_neighbours

[pre-commit.ci] auto fixes from pre-commit.com hooks

69cda1a

for more information, see https://pre-commit.ci

arrange builders

fddc027

Merge branch 'feat/spatial_neighbours' of https://github.com/selmanoz…

a30988a

…leyen/squidpy into feat/spatial_neighbours

selmanozleyen and others added 6 commits April 2, 2026 14:09

better behaviour

f194b10

move classes

ae1eba1

Merge branch 'main' into feat/spatial_neighbours

a86572b

cleanup

039b852

Merge branch 'feat/spatial_neighbours' of https://github.com/selmanoz…

4699993

…leyen/squidpy into feat/spatial_neighbours

[pre-commit.ci] auto fixes from pre-commit.com hooks

b5dcf9f

for more information, see https://pre-commit.ci

selmanozleyen requested a review from grst April 2, 2026 12:30

selmanozleyen added 5 commits April 2, 2026 14:36

add docs

4c11adf

Merge branch 'feat/spatial_neighbours' of https://github.com/selmanoz…

2b937e9

…leyen/squidpy into feat/spatial_neighbours

remove leftover

ea1ce93

resolution in reolve func

ebb7fd5

reduce dup code

69a9394

selmanozleyen requested a review from timtreis April 2, 2026 12:53

selmanozleyen self-assigned this Apr 2, 2026

grst reviewed Apr 2, 2026

View reviewed changes

selmanozleyen and others added 5 commits April 2, 2026 17:11

deprecate invalid n_neighs

77755ed

expose new functions and deprecate the flat one

d8a8145

move branches to classes

2e75b3f

[pre-commit.ci] auto fixes from pre-commit.com hooks

8c1c2ef

for more information, see https://pre-commit.ci

remove radius abstraction

268d30c

timtreis reviewed Apr 11, 2026

View reviewed changes

timtreis requested changes Apr 11, 2026

View reviewed changes

selmanozleyen and others added 16 commits April 13, 2026 07:46

mark as TODO's

5a02b59

rename sugg

3af98cc

add combine into API

e8a19d7

[pre-commit.ci] auto fixes from pre-commit.com hooks

ac682a3

for more information, see https://pre-commit.ci

update docs

0f3329c

[pre-commit.ci] auto fixes from pre-commit.com hooks

00ec2b0

for more information, see https://pre-commit.ci

Functions to QC histopathology images (scverse#1036)

1a87841

remove coord_type abstraction

1cb6fde

[pre-commit.ci] auto fixes from pre-commit.com hooks

8afcd03

for more information, see https://pre-commit.ci

Merge branch 'main' into feat/spatial_neighbours

3c4dffb

make builder positional consistently

21f02c3

make more abstractions

744c757

[pre-commit.ci] auto fixes from pre-commit.com hooks

dbc7f0d

for more information, see https://pre-commit.ci

graphmatrixT exposure

d7e5e5f

[pre-commit.ci] auto fixes from pre-commit.com hooks

13768e2

for more information, see https://pre-commit.ci

selmanozleyen requested a review from timtreis April 13, 2026 22:33

shashkat reviewed Apr 20, 2026

View reviewed changes

Merge branch 'main' into feat/spatial_neighbours

ea3f3ca

shashkat mentioned this pull request Apr 21, 2026

n-hop neighbors in spatial graph #1159

Open

grst reviewed Apr 22, 2026

View reviewed changes



		def _prepare_spatial_neighbors_input(
		adata: AnnData \| SpatialData,

		@@ -0,0 +1,440 @@
		"""Graph construction strategies for spatial neighbor graphs.

		if isinstance(radius, tuple):
		postprocessors.append(DistanceIntervalPostprocessor(tuple(sorted(radius))))

Conversation

selmanozleyen commented Apr 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov Bot commented Apr 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

grst left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

timtreis Apr 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shashkat left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

selmanozleyen commented Apr 2, 2026 •

edited

Loading

codecov Bot commented Apr 2, 2026 •

edited

Loading

timtreis Apr 11, 2026 •

edited

Loading