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.

timtreis · 2026-09-18T11:09:40Z

I think it'd be nice to add a "share_niches: bool" parameter to the entire function that, if library_key is preset, decides whether things are jointly modeled. But that'd go beyond the scope of this PR. That's iirc also what official Cellcharter does

## mask `mask` came from monkeybread's `cellular_niches`, where rows are dropped before the clustering graph is built. `6638075b` ported that; `b8dff5de` replaced it with a post-hoc relabel, so everything is clustered now and masked rows are renamed afterwards. Labels on the kept cells, against an unmasked run: v1.8.3 35% masked cells absent from the fit main 100% the mask has no effect on the model at all here 20% 100% is the tell. The docstring has promised the exclusion since 2024 with no flavor qualifier, and on v1.8.3 it held for neighborhood (0/20 masked cells got a niche) and was false for the other three (20/20) -- silently, because `mask` is in no flavor's `unused` list. Renamed `cluster_mask`, because the name should say which stage it restricts: the profile is built from every observation, so a masked one still reaches its neighbors and is absent only from the clustering. Applied by the three flavors that build an embedding. spatialleiden raises rather than accepting one it cannot honour: it clusters the graphs, so an observation either takes part or loses its edges, and `is_membership_fixed` is never passed to `optimise_partition_multiplex`. v1.8.3 did nothing there and did not warn, so no working call breaks. `_fitted_on` aligns the mask in one place and fixes two things: a mask that omits observations now keeps them, which is how the documented three-entry example reads and which used to raise `IndexingError` on main and `IndexError` on v1.8.3, so the documented usage has never worked; and a mask sharing no index value, or excluding everything, raises. ## Follow-ups One library loop instead of two. `_stratify` is the loop both pipelines had their own copy of, called with a `run_one` callback; 104 records byte-identical across the refactor. spatialleiden keeps its own `run_one` rather than becoming a Clusterer, because it takes two graphs and no feature matrix. `key_added` on all four flavors, defaulting to the existing column names. This retires the private `prefix`, which existed only to plumb the per-library label through the recursion `_stratify` replaced. A warning when a graph has edges across libraries under `library_key`. The loop slices `obsp`, which only preserves a block-diagonal graph, and nothing checked. spatialleiden checks both its graphs, and its `latent_connectivities_key` defaults to whatever `scanpy.pp.neighbors` left in `connectivities`, which knows nothing of libraries. `min_niche_size` was also listed as unused by every flavor while all four apply it. `use_rep` on utag, which could emit an embedding but not take one back, so its PCA sat inside the per-library loop and each library got its own basis -- widths need not even match (50 columns in one library, 7 in another). `'X'` now spells `adata.X` on both `use_rep` arguments. `_aggregate_over` scales the rows after the sum rather than normalizing the adjacency first, which rounded 1/k to float32 and left a representable mean inexact. A non-floating product is promoted first, since the CellCharter rings are bool and an integer X summed to an integer that cannot hold the quotient. Dividing by the signed row sum also fixes the weighted mean for negative edge weights. 153 tests.

Stratifying gives every library its own embedding and its own clustering, so labels carry a lib=<id>_ prefix and a niche in one library is unrelated to the same-numbered niche in another. Say so where users read it, and point at the alternative: no library_key, a batch-corrected use_rep and a graph built with spatial_neighbors(library_key=...). The Returns sections promised the embedding in .obsm even when stratifying, where nothing is written because the per-library embeddings share no axes.

grst · 2026-09-18T11:38:40Z

IMO the neighborhood graph should respect ~~niches~~ libraries, but the embedding+clustering should be across samples. That's the whole point, to classify cells into shared labels that can be used to compare between groups of samples.

shashkat · 2026-09-18T12:27:39Z

I think with the library_key argument, it is a bit confusing for end user to understand how library_key interacts with the batch-aware niche clustering of cellcharter, because as grst mentioned, cellcharter's whole point is to compute niches shared across samples. Having the share_niches arg could be good, as it allows for both options, and the documentation of this arg will help the user understand what library_key is actually doing, but I am not sure when someone would need share_niches to be false when using calculate_niche_cellcharter, as likely they would be using calculate_niche_cellcharter because they want to perform a combined niche clustering of the data which probably has batches.

Just to reiterate, even though cellcharter.gr.aggregate_neighbors has a sample_key arg, it's not necessary as the spatial graph should already encode the information about batches and hence aggregation will be batch-aware. So I believe library_key isn't a necessary arg for calculate_niche_cellcharter and can be removed.

selmanozleyen · 2026-09-18T13:06:28Z

I think it'd be nice to add a "share_niches: bool" parameter to the entire function that, if library_key is preset, decides whether things are jointly modeled. But that'd go beyond the scope of this PR. That's iirc also what official Cellcharter does

Where does cellcharter provide something like this?

IMO the neighborhood graph should respect niches, but the embedding+clustering should be across samples. That's the whole point, to classify cells into shared labels that can be used to compare between groups of samples.

Is this in general? If so I don't see a reason to require library_key then (for other flavors except spatialleiden)? because all the embedding + clustering is already across samples by default

timtreis · 2026-09-18T21:15:06Z

I think it'd be nice to add a "share_niches: bool" parameter to the entire function that, if library_key is preset, decides whether things are jointly modeled. But that'd go beyond the scope of this PR. That's iirc also what official Cellcharter does

Where does cellcharter provide something like this?

It doesn't provide the option, it just jointly models it by default.

IMO the neighborhood graph should respect niches, but the embedding+clustering should be across samples. That's the whole point, to classify cells into shared labels that can be used to compare between groups of samples.

Is this in general? If so I don't see a reason to require library_key then (for other flavors except spatialleiden)? because all the embedding + clustering is already across samples by default

Yeah, I agree. I also wouldn't require it, the baseline assumption should be the thing that makes the most sense (joint niches here)

selmanozleyen · 2026-09-19T00:14:37Z

Ok any reason not to remove it then for utag, cellcharter, neighbours? Because slice and run is all we do there and we can just document that if someone really wants it. Because library_key is a confusing arg here atm IMO

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

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 8e59c35 to 9f01a12 Compare September 18, 2026 11:32

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

Comment thread src/squidpy/_docs.py Outdated

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

Open

		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!

timtreis commented Sep 18, 2026

Uh oh!

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

Uh oh!

Uh oh!

shashkat commented Sep 18, 2026

Uh oh!

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

Uh oh!

Uh oh!

timtreis commented Sep 18, 2026

Uh oh!

selmanozleyen commented Sep 19, 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

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

grst commented Sep 18, 2026 •

edited

Loading

selmanozleyen commented Sep 18, 2026 •

edited

Loading

selmanozleyen commented Sep 24, 2026 •

edited

Loading