fix: reproducible niche Leiden clustering + expose flavor/random_state/n_iterations - #1266
fix: reproducible niche Leiden clustering + expose flavor/random_state/n_iterations#1266timtreis wants to merge 7 commits into
Conversation
The LocalCluster(processes=True) path could leave worker/nanny processes alive after teardown under Python 3.14. An orphan inheriting stdout/stderr keeps the pipe open, hanging consumers that wait for EOF (CI log capture). Track the cluster's own children and reap any survivor after close(). Closes #1267
Bind the worker-reaping teardown to LocalCluster creation via a reusable context manager instead of inlining it in _run_tiled, and route the test's duplicate cluster site through it so both share the config and the guaranteed teardown.
calculate_niche_neighborhood/_utag silently dropped random_state and n_iterations on the Leiden path and used the deprecated leidenalg backend, so niche labels were unseeded and unstable across leidenalg/igraph versions (#1260, integration CI). Expose flavor/n_iterations/random_state on both functions, thread them through _LeidenClusterer and the deprecated umbrella, and default to the igraph backend with a fixed seed. Guard the spatialleiden tests with importorskip and regenerate the pinned niche labels. Closes #1260
Leiden partitions of the tiny toy fixture are not stable across igraph/leidenalg versions (multiple equal-modularity optima), so hardcoded labels were flaky in CI (#1260). Assert squidpy's contract instead: every cell is assigned, library_key stratification prefixes per library, mask/min_niche_size postprocessing behaves, and a fixed random_state is reproducible.
1 <= nunique <= n_obs is always true once every cell is asserted assigned, so it locked in nothing.
2b54f6c to
184fbf0
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1266 +/- ##
=======================================
Coverage 78.60% 78.60%
=======================================
Files 63 63
Lines 9348 9353 +5
Branches 1559 1559
=======================================
+ Hits 7348 7352 +4
Misses 1426 1426
- Partials 574 575 +1
🚀 New features to boost your workflow:
|
| (the ``'leidenalg'`` backend is deprecated in scanpy). | ||
| n_iterations | ||
| Number of Leiden iterations. ``-1`` iterates until convergence. | ||
| random_state |
There was a problem hiding this comment.
should we call this perhaps a seed? Because it would be nice to have consistency here no? I will also rename the other random_state appearance in compute_niche to seed before a release as well
There was a problem hiding this comment.
I think I got random_state from scanpy/sklearn - ideally we'd use the same. But even internally, Squidpy is inconsistent right now. I'm fine with whatever but I'd learn towards ecosystem consistency.
There was a problem hiding this comment.
@flying-sheep @ilan-gold any plans on changing scanpys random_state any time soon?
There was a problem hiding this comment.
I don't think they will change it since Phil just wrote a legacy support but we only use random_state under our experimental module and the calculate_niche which I plan to fix. I know scanpy takes it but I think it's better if we control the seed perhaps.
There was a problem hiding this comment.
I do think we should control the seed but I feel like ideally as many scverse packages share the same keywords for the same things, no?
There was a problem hiding this comment.
Okay, but then let's look forward and just align everything to the new target pattern (with a dep warning on everything else until we move to 2.0) no? Why do it twice if we can just adapt the new standard now (not necessarily in this PR but in the very near feature).
There was a problem hiding this comment.
https://scanpy.scverse.org/en/latest/api/generated/scanpy.tl.umap.html
wild that this isn’t in a stable release, feels like I did this forever ago
There was a problem hiding this comment.
Why do it twice if we can just adapt the new standard now (not necessarily in this PR but in the very near feature).
Yeah I agree, I am forrngs but didn't think you'd care or be high priority. But one thing is for sure, we shouldn't introduce random_state. rng's itself can be another topic/PR
There was a problem hiding this comment.
I only care because it surfaced as a point of discussion now and I'd rather deal with it now'ish or we'll forget about it and do it in 3 years 😅 So in this PR we go with seed to be consistent with the rest of Squidpy and then ASAP move to the new rng logic?
There was a problem hiding this comment.
So in this PR we go with seed to be consistent with the rest of Squidpy and then ASAP move to the new rng logic?
Internally I try to do this as much as possible. The only thing I use seed in ligrec for example is to create an rng first in the beginning. So it should be straightforward to do all these breaking changes at once. Or basically we can give a warning saying: hey seed here is only being used to create np.default_rng(seed) atm seed will be dropped in the future etc.
I'd rather deal with it now'ish or we'll forget about it and do it in 3 years
Ok then let's merge #1270 first then we can do all the public facing breaking changes in one PR
Merges #1266 (fix/issue-1260), which makes the niche Leiden path reproducible and defaults it to the igraph backend. Reconciled with this branch's seeding convention: - `random_state` renamed to `seed` throughout, since 777449e already replaced it here and `calculate_niche` now raises on `random_state`. - `_LeidenClusterer` derives a per-resolution generator via `spawn_generators` instead of reusing one flat int, matching what `calculate_niche_spatialleiden` already does. Adding a resolution no longer shifts the others. - Kept the pinned cellcharter labels: it clusters with a GMM, not Leiden, so #1260's "labels drift across backends" rationale does not apply. Two further fixes: - `calculate_niche_cellcharter` failed with "blocks must be 2-D" whenever `adata.X` was dense, because `scipy.sparse.hstack` needs at least one sparse block. Stack densely; the result was densified anyway. - A collapsed mixture component aborted a whole `sweep_auto_k` with sklearn's bare message. Re-raise naming the K and `reg_covar`.
3 participants
Fixes the squidpy integration-CI failures in
tests/graph/test_niche.py(#1260).Two root causes:
calculate_niche_neighborhood/_utagsilently droppedrandom_state/n_iterationson the Leiden path and used the deprecatedleidenalgbackend, so niche labels were unseeded and unstable acrossleidenalg/igraphversions (theassert np.False_failure).spatialleidentests hard-failed withImportErrorwhen the optional dep was absent.Changes:
flavor(default"igraph"),n_iterations(default-1),random_state(default0) oncalculate_niche_neighborhood/calculate_niche_utag; thread them through_LeidenClustererand the deprecatedcalculate_nicheumbrella (where they were previously marked "unused").igraphwith a fixed seed -> reproducible labels. Changes default niche labels vs earlier releases.spatialleidentests withimportorskip; regenerate the pinned exact labels for the nhood/utag/library/multipostprocessor tests.Closes #1260