Incorporating feedback on niche refactor by shashkat · Pull Request #1284 · scverse/squidpy

shashkat · 2026-09-01T02:59:14Z

Description

Big thanks to @grst for invaluable feedback (#1277) on the niche refactor in #1245. Incorporating those suggestions in this PR.

…m and store the niche embedding in adata.obsm now. Not sure how much computational benefit this offers, as now, instead of many small embeddings (for each library-subsetted adata), one big embedding is being computed (for the whole adata). But irrespective of computational advantage, I think it can be useful to store the embedding in adata.obsm to inspect for end user.

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

…t layer of adata and ii) no longer creating temporary adata object in _UtagEmbedder

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

codecov · 2026-09-04T03:46:30Z

Codecov Report

❌ Patch coverage is 82.19178% with 65 lines in your changes missing coverage. Please review.
✅ Project coverage is 78.57%. Comparing base (8286274) to head (5de58fb).

Files with missing lines	Patch %	Lines
src/squidpy/gr/_nhood.py	70.25%	40 Missing and 7 partials ⚠️
src/squidpy/gr/_niche.py	91.71%	9 Missing and 5 partials ⚠️
src/squidpy/gr/_clusterers.py	87.09%	3 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1284      +/-   ##
==========================================
+ Coverage   78.44%   78.57%   +0.13%     
==========================================
  Files          63       64       +1     
  Lines        9532     9598      +66     
  Branches     1594     1612      +18     
==========================================
+ Hits         7477     7542      +65     
- Misses       1489     1498       +9     
+ Partials      566      558       -8

Files with missing lines	Coverage Δ
src/squidpy/_docs.py	`95.45% <100.00%> (+0.10%)`	⬆️
src/squidpy/gr/neighbors.py	`91.47% <100.00%> (-0.19%)`	⬇️
src/squidpy/gr/_clusterers.py	`87.09% <87.09%> (ø)`
src/squidpy/gr/_niche.py	`86.69% <91.71%> (+8.26%)`	⬆️
src/squidpy/gr/_nhood.py	`67.33% <70.25%> (+1.05%)`	⬆️

... and 1 file with indirect coverage changes

🚀 New features to boost your workflow:

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

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

…cells are not re-visited

…her cleaning of _NhoodProfileEmbedder

…ation of hop adj matrices (_compute_hop_adjacency_matrices function) and removing unnecessary adata creation

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

…ces from _CellcharterEmbedder

…n _NhoodProfileEmbedder

… the result columns back to categoricals in _postprocess_niche_results, iii) explained resolutions arg better in calculate_niche_neighborhood

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

… Also renamed the CellcharterEmbedder to a more generic name

…te_niche_cellcharter

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

shashkat · 2026-09-10T04:38:21Z

Hi, @grst and @selmanozleyen, this is ready for review. I think the suggestions were all quite useful and some of them I also thought about during the refactor, but didn't implement at that time. Here are some questions/points-of-discussion from me about the same:

Right now, for the warnings, I am switching to logg, but I wanted to ask what would be the recommended approach to do logging. In _build.py too, I see both, logg and warnings used. One thing I noticed that even though the logg outputs look nicer, the logg.warning outputs don't appear when we run tests.
I have formalized the hopping mechanism to be able to get an adjacency matrix of first-time-visited-nodes at hop n, like it was being attempted in CellcharterEmbedder. This I implemented in function _compute_hop_adjacency_matrices, and since there was an argument for hopping also in NhoodProfileEmbedder, I used _compute_hop_adjacency_matrices there too. However, this lead to the hopping mechanism becoming slightly different from previous in NhoodProfileEmbedder. Previously for hopping in NhoodProfileEmbedder, just the adjacency matrix was being multiplied to itself, which would not account for degree, and lead to I believe unexpected aggregation. Now, that doesn't happen, and degree is accounted for.
I think this formalization of hopping mechanism can be useful in the implementation for n-hop neighbors in spatial graph #1159 too.

It cannot build a graph. The flavors take spatial_connectivities_key, never spatial_key, and call no builder, so all library_key can do is subset a graph somebody else built. On a graph already built with spatial_neighbors (library_key=...) that subsetting is a no-op, measured to 0.00e+00; on one built pooled it drops the crossing edges without replacing them, leaving 198/200 cells short at least one neighbour where a genuine per-library run gives every cell its full degree. So it either does nothing or degrades, and never does the thing its name suggests. What is left is fitting a separate model per library, which makes the labels of one library unrelated to another's: three identical sections at n_clusters=4 give 12 labels with none shared, against 4 shared without it. The point of running across samples is a shared vocabulary, so that is now the only behaviour for neighborhood, utag and cellcharter. spatialleiden keeps it. It has no embedding step and both its graphs are inputs, so none of the above applies, and one multiplex run over pooled sections under-clusters through the global null model. calculate_niche is released with the parameter, so it raises for the three flavors that no longer take one rather than silently pooling, and still forwards it for spatialleiden. The loop leaves the pipeline with it. _stratify was unwrapping the SpatialData, looping over libraries and putting the table back; the first and last are what every path needs, so they are _on_table now and _stratify is only the loop on top. calculate_niche_custom runs once over everything and takes neither library_key nor graph_keys. spatialleiden calls the loop when library_key is given and the plain pipeline when it is not, so the stratified shape is opt-in rather than the frame every flavor is built inside. Two aggregation bugs surfaced on the way, both older than this change: Integer features lost their ring means. The block the rings are written into took the features' dtype, so an integer X through use_rep truncated every mean back to a count -- 0 of 240 ring entries kept a fractional part. Bool features aggregated to 1/k in every column, max error 0.75 against a float64 reference, because `bool @ bool` saturates to True instead of summing and the widening happened to the product rather than the operand.

shashkat

Other than these things, it looks good to me!

Just one more thing: fast-array-utils.types has HasArrayNamespace only from v1.5 onwards. So we may need to add the corresponding pin in the pyproject.yaml file.

grst

First round of comments, for now focussing on _nhood.py. Still need to take a look at _niche.py.

grst · 2026-09-24T06:48:54Z

    return group_offsets, group_indices
+
+
+@njit(inline="always", cache=True)


Have you benchmarked if the numba approach is actually faster than the matrix multiplication approach?

yes I did, I will do a before after once the pr is more ready to be merged

grst · 2026-09-24T06:57:46Z

+    """Disjoint adjacency rings, one per hop up to *max_hop*.
+
+    Ring ``k`` holds the pairs first reached at hop ``k + 1``, so the rings never restate
+    each other. Ring 0 is the input as booleans, self-loops included.


@marcovarrone, reaching out to you as one of the original cellcharter authors here.

Context: we are working on native implementation of common niche-calling algorithms in squidpy, including cell-charter. The plan is to make the implementation modular by decomposing the neighborhood embedding and the clustering step, as these are often shared between different approaches.

I understand cellcharter builds an embedding by aggregating the spatial neighborhood at different levels. What I'm unsure: did you intend to aggregate growing circles, or disjoint rings? I.e. does the layer 2 include all cells from layer1 and so on?

grst · 2026-09-24T07:14:46Z

+def _power_adjacencies(adj: CSBase, max_hop: int) -> list[CSBase]:
+    if max_hop < 1:
+        raise ValueError(f"max_hop must be >= 1, got {max_hop}.")
+
+    adjacencies, power = [adj], adj
+    for _ in range(1, max_hop):
+        power = power @ adj
+        adjacencies.append(power)
+    return adjacencies


Just one thought, but that's more likely for a future iteration: The n-hop adjacency matrices can become quite large as the sparsity reduces... Maybe we can come up with a smarter solution (maybe numba-based) that doesn't require to keep the entire adjaceny matrix in memory at the same time.

grst · 2026-09-24T07:22:39Z

+    if aggregation == "variance":
+        mean = to_dense(mean_over(features))
+        dense = to_dense(features)
+        return to_dense(mean_over(dense * dense)) - mean * mean


... and possibly fast_array_utils.stats.mean_var?

selmanozleyen · 2026-09-24T07:44:41Z

Thanks for the reviews guys! After talking to @timtreis yesterday I realized spatiallieden doesn't also need library_key, so updates are on its way

No flavor takes library_key now. spatialleiden builds neither of its graphs, so per-library graphs were already the caller's job, and what library_key added on top was one Leiden run per library, a separate label space for each: what the other three flavors already dropped. The deprecated calculate_niche keeps the parameter and raises for every flavor, naming the alternative. _stratify, the cross-library warning and the lib=<id>_ prefix go with it. The neighborhood profile counts each reached cell once again. Past hop 1 it read walk counts off the matrix power, so a cell reached by two paths counted twice and a cell took a flat 1/d share of its own 2-hop profile. Hop 1 keeps the edge weights; past it only reachability is defined. Both flavors warn on a weighted graph. Pins that a lower-degree cell gets its own composition, which v1.8.3's padding got wrong. From the review: - fast-array-utils>=1.5, for types.HasArrayNamespace - LeidenClusterer.labels_ is .cat.codes; the niche labels are unchanged - _bfs_shells reads get_num_threads() instead of taking n_threads - unlabelled observations come from groups.notna(), not the one-hot sums Also: - the neighbor count takes that mask instead of every hop matrix being copied with it, including under aggregation="sum" where it changed nothing; bit-identical, and the 3-hop profile is ~25% faster - the BFS no longer writes ring 0 only for it to be replaced by the input - min_niche_size applies before the categorical is built; spatialleiden reads its labels back inside its own loop, and they are strings now as on every other flavor, instead of integers unless min_niche_size was set - niche categories sort numerically, 0, 1, 2 ... 10, as sc.tl.leiden has them, instead of as text

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

calculate_niche is released with library_key, and raising broke every v1.8.x script that passed it. It now warns and keeps v1.8.3's behaviour until the function itself goes in v1.9.0: each library fitted on its own subset, labels prefixed lib=<id>_, observations with no library left as not_a_niche. The loop is its own function, _niches_per_library, marked for deletion with calculate_niche, which is its only caller. The flavor functions do not take library_key and do not go through it; the umbrella runs the flavor on each library's subset and merges the labels back. The columns it merges come from each flavor's key_added and _resolution_values, so a rerun overwrites them rather than keeping the previous run's labels.

timtreis

we had already discussed most details, in particular the library_key situation. lgtm

The library_key warning and docstring said to call the flavor on each library's subset without showing how, and did not say which suggestion answers which need. They now separate the two: graphs built per library either way, library_key unset for niches shared across libraries, and a slice per library for niches per library. The docstring example copies the labels back, which a loop over slices otherwise discards; run on two libraries it gives the same 300/300 labels as the deprecated path.

selmanozleyen · 2026-09-25T18:10:01Z

I will merge this now and assess the open items in another pr, thanks guys!

After #1284: calculate_niche_custom takes only data positionally. The deprecated calculate_niche and its validator keep main's signatures and are exempted, since they are removed in v1.9.0; decorating the umbrella also moved its deprecation warning off the caller. The two numba kernels #1284 added are exempted like the others.

Rebuilt on #1284's niche refactor, reusing its pieces instead of keeping parallel ones: main's Clusterer and LeidenClusterer, calculate_niche_custom, the hop shells and nhood_aggregate. New on top of them: - sweep_auto_k and AutoKClusterer (gr/_autok.py), with ClusterAutoKResult next to them; SweepableClusterer extends main's Clusterer with the number-of-clusters parameter the sweep sets per fit - gr.cluster_auto_k and gr.cluster_stability - calculate_niche_cellcharter takes a (min, max) tuple or a sequence for n_clusters and then sweeps, with max_runs, convergence_tol and store_labels; _fit_clusterers writes an estimator's extra labelings and diagnostics when it leaves them - gr.nhood_entropy, the entropy of nhood_aggregate's neighborhood composition

Rebuilt on scverse#1284's niche refactor, reusing its pieces instead of keeping parallel ones: main's Clusterer and LeidenClusterer, calculate_niche_custom, the hop shells and nhood_aggregate. New on top of them: - sweep_auto_k and AutoKClusterer (gr/_autok.py), with ClusterAutoKResult next to them; SweepableClusterer extends main's Clusterer with the number-of-clusters parameter the sweep sets per fit - gr.cluster_auto_k and gr.cluster_stability - calculate_niche_cellcharter takes a (min, max) tuple or a sequence for n_clusters and then sweeps, with max_runs, convergence_tol and store_labels; _fit_clusterers writes an estimator's extra labelings and diagnostics when it leaves them - gr.nhood_entropy, the entropy of nhood_aggregate's neighborhood composition

Rebuilt on #1284's niche refactor, reusing its pieces instead of keeping parallel ones: main's Clusterer and LeidenClusterer, calculate_niche_custom, the hop shells and nhood_aggregate. New on top of them: - sweep_auto_k and AutoKClusterer (gr/_autok.py), with ClusterAutoKResult next to them; SweepableClusterer extends main's Clusterer with the number-of-clusters parameter the sweep sets per fit - gr.cluster_auto_k and gr.cluster_stability - calculate_niche_cellcharter takes a (min, max) tuple or a sequence for n_clusters and then sweeps, with max_runs, convergence_tol and store_labels; _fit_clusterers writes an estimator's extra labelings and diagnostics when it leaves them - gr.nhood_entropy, the entropy of nhood_aggregate's neighborhood composition

shashkat and others added 5 commits August 31, 2026 22:49

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

33a050f

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

incorporated suggestions in utag flavor: i) now can supply a differen…

c305e3b

…t layer of adata and ii) no longer creating temporary adata object in _UtagEmbedder

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

1e7e388

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

fixed docs

c5236b0

shashkat and others added 22 commits September 5, 2026 15:18

improved implementation of calculate_nhood_profile in NhoodEmbedder

3e776a2

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

46b0746

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

remove import

443670c

fixed _hop function to account for node degree when ensuring visited …

4aced55

…cells are not re-visited

removed unnecessary .tolil() and .tocsr() in _setdiag function

80e2e01

dont convert output to df in _calculate_neighborhood_profile() and ot…

5aaee14

…her cleaning of _NhoodProfileEmbedder

improved implementation of _CellcharterEmbedder by formalizing comput…

4867b51

…ation of hop adj matrices (_compute_hop_adjacency_matrices function) and removing unnecessary adata creation

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

2137bd1

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

remove import

98ef18c

added tests for _compute_hop_adjacency_matrices

1bdee56

avoid duplication of matrix when calling _compute_hop_adjacency_matri…

fadb084

…ces from _CellcharterEmbedder

use standardized hopping function _compute_hop_adjacency_matrices() i…

0eb6024

…n _NhoodProfileEmbedder

keep the warning style to consistently use logg instead of warnings.warn

e951e9e

throw error when n_hop_weights < distance in NhoodProfileEmbedder

7def18b

i) made only data as positional arg in user-facing funcs, ii) convert…

ad06f27

… the result columns back to categoricals in _postprocess_niche_results, iii) explained resolutions arg better in calculate_niche_neighborhood

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

cff1100

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

updated documentation of user-facing functions to be more descriptive

e72b45e

move use_rep from CellcharterEmbedder to calculate_niche_cellcharter.…

9b7a885

… Also renamed the CellcharterEmbedder to a more generic name

remove n_components arg from _NHopPCAEmbedder

c1fb326

made it not necessary to have sparse expression matrix to use calcula…

8e08465

…te_niche_cellcharter

replaced sc.tl.pca with sc.pp.pca as recommended API

f6c5c15

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

7f6b8c9

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

shashkat marked this pull request as ready for review September 10, 2026 04:36

selmanozleyen force-pushed the niche_refactor_suggestions branch from 2c83ebe to 6676965 Compare September 23, 2026 14:35

shashkat commented Sep 24, 2026

View reviewed changes

Comment thread src/squidpy/gr/_clusterers.py Resolved

Comment thread src/squidpy/_docs.py Resolved

grst reviewed Sep 24, 2026

View reviewed changes

selmanozleyen and others added 2 commits September 25, 2026 13:17

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

5de58fb

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

selmanozleyen requested a review from timtreis September 25, 2026 11:49

timtreis approved these changes Sep 25, 2026

View reviewed changes

selmanozleyen merged commit 69a3bea into scverse:main Sep 25, 2026
12 of 13 checks passed

selmanozleyen mentioned this pull request Sep 25, 2026

Make most arguments keyword-only, enforced by PLR0917 (#1288) #1297

Merged

selmanozleyen added the release-added label Oct 29, 2026

		return group_offsets, group_indices


		@njit(inline="always", cache=True)

Conversation

shashkat commented Sep 1, 2026 • edited by selmanozleyen Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Uh oh!

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

Uh oh!

Codecov Report

Uh oh!

shashkat commented Sep 10, 2026

Uh oh!

shashkat left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

grst left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

grst Sep 24, 2026

Choose a reason for hiding this comment

Uh oh!

selmanozleyen Sep 25, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

grst Sep 24, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

grst Sep 24, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

grst Sep 24, 2026

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Uh oh!

timtreis left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

selmanozleyen commented Sep 25, 2026

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

shashkat commented Sep 1, 2026 •

edited by selmanozleyen

Loading

codecov Bot commented Sep 4, 2026 •

edited

Loading

selmanozleyen commented Sep 24, 2026 •

edited

Loading