fix: reproducible niche Leiden clustering + expose flavor/random_state/n_iterations - #1266
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.74% 78.75%
=======================================
Files 63 63
Lines 9362 9370 +8
Branches 1557 1558 +1
=======================================
+ Hits 7372 7379 +7
Misses 1421 1421
- Partials 569 570 +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`.
* [pre-commit.ci] pre-commit autoupdate updates: - [github.com/biomejs/pre-commit: v2.5.7 → v2.5.8](biomejs/pre-commit@v2.5.7...v2.5.8) - [github.com/tox-dev/pyproject-fmt: v2.27.0 → v2.28.0](tox-dev/pyproject-fmt@v2.27.0...v2.28.0) - [github.com/astral-sh/ruff-pre-commit: v0.16.2 → v0.16.3](astral-sh/ruff-pre-commit@v0.16.2...v0.16.3) - [github.com/zizmorcore/zizmor-pre-commit: v1.24.1 → v1.29.0](zizmorcore/zizmor-pre-commit@v1.24.1...v1.29.0) * [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> Co-authored-by: Tim Treis <tim.treis@helmholtz-muenchen.de>
* Rename niche random_state `random_state` is dropped outright rather than deprecated: it is simply gone from the signatures, so passing it raises Python's own TypeError. The released defaults are preserved -- `seed` still defaults to 42, so calls that do not pass it stay reproducible exactly as before. Passing `seed=None` opts out. Internally `seed` now feeds `numpy.random.Generator`s: `spawn_generators` derives an independent generator per library and per resolution, and `rng_to_random_state` converts at the boundary of third-party APIs that take an int but not a Generator (scikit-learn, spatialleiden). Adding a library or a resolution therefore no longer shifts the others. Derived from 777449e on feat/cluster-auto-k. * Rename experimental.im random_state Completes the rename, so `random_state` no longer appears as a squidpy parameter name anywhere. `WekaParams.random_state`, `VahadaneParams.random_state` and `_refine_with_background_classifier`'s parameter become `seed`, keeping their existing default of 0. No conversion helper is needed: these are plain `int | None` and go straight into scikit-learn, which accepts that. The `random_state=` keywords that remain are scikit-learn's own, on `RandomForestClassifier` and `NMF`. * Flip unreleased seed defaults `seed` defaults to `None` on everything that has not shipped yet: `calculate_niche_cellcharter`, `calculate_niche_spatialleiden`, `WekaParams`, `VahadaneParams` and `_refine_with_background_classifier`. A new API defaulting to a fixed seed hides non-determinism behind an arbitrary constant; `None` makes the choice explicit and matches the rest of `squidpy.gr`. `calculate_niche` keeps its released default of 42. * Adopt SPEC 7 rng `seed` and `random_state` become `rng`, accepting a seed, a `numpy.random.Generator` or `None`, per SPEC 7. The old names still work and emit a `FutureWarning` naming what happens to the value: it now seeds a generator rather than reaching the underlying library as a legacy `random_state`, so results for a given value can differ. Public entry points normalise once with `numpy.random.default_rng`; everything downstream takes a `Generator`. `spawn_generators` is gone -- after that split it was a one-line wrapper around `Generator.spawn`. The one internal still seeing a raw `rng` is `_validate_niche_args`, which reports on what the caller passed and needs `None` to stay `None`. `_segment_weka` also stops handing the same seed to both the random forest and the refinement classifier; they now draw from one generator. * Unroll rng type aliases `SeedLike` and `RNGLike` become plain unions instead of PEP-695 `type` statements. A `type` statement builds a `TypeAliasType`, which sphinx deliberately renders by name -- so `VahadaneParams.rng` documented itself as `SeedLike | RNGLike | None`, two names that resolve to nothing because `squidpy._utils` is private and undocumented. Plain unions are evaluated, so autodoc expands them to `int | integer | Sequence[int] | SeedSequence | Generator | BitGenerator | None` on attributes, matching what sphinx-autodoc-typehints already produced for function parameters. The aliases no longer appear as names anywhere, so the build is nitpick-clean without ignore entries for them. * Rename rng_to_random_state to legacy_random for clarity and update test fixture to use a seed value directly * Rename deprecated_rng_param to deprecated_randomness_param and update references to legacy_random * Replace 'generator' with 'rng' for consistency in niche calculations and GMM clustering * Update src/squidpy/gr/_niche.py --------- Co-authored-by: Philipp A. <flying-sheep@web.de>
#1274) * fix: update niche function parameters to use 'copy' instead of 'inplace' for better clarity * add import * add import again resulting from the merge conflict * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * fix the conflict --------- Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
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