chore: switch to rngs - #1270
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1270 +/- ##
==========================================
+ Coverage 78.60% 78.72% +0.11%
==========================================
Files 63 63
Lines 9348 9371 +23
Branches 1559 1561 +2
==========================================
+ Hits 7348 7377 +29
+ Misses 1426 1423 -3
+ Partials 574 571 -3
🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Everything should be rng: SeedLike | RngLike | None = None, not seed: Something = 0, with the behavior and types specified here: https://scientific-python.org/specs/spec-0007/
Incompatible 3rd party APIs that only take a seed should be seeded via third_party_api(seed=rng.bytes(16)) (in case the 3rd party API takes a 128 bit seed sequence)
For backwards compatibility, you could do the same as I did for scanpy, i.e. if nothing is passed, add a decorator that passes a compatibility Generator which behaves exactly as a RandomState. New APIs (i.e. ones that haven’t been released yet or future ones) should have the standard None default, i.e. behave nondeterministically.
|
you know what, while I am at it I might as well do the full switch here |
`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.
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`.
`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.
`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.
e05e252 to
a2095a5
Compare
`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.
b135863 to
cbba8b9
Compare
selmanozleyen
changed the title
Yeah, why rename these twice? Once to |
flying-sheep
left a comment
flying-sheep
left a comment
There was a problem hiding this comment.
Looks great! If you don’t have to maintain backwards compat, this is exactly how is should be done. I only have some nitpicks, mostly about naming.
|
|
||
| %(seed_versionchanged)s | ||
|
|
||
| %(rng_versionchanged)s |
There was a problem hiding this comment.
are there other places where this should go? Also, is %(seed_versionchanged)s above still relevant?
There was a problem hiding this comment.
yes it is relevant because I fixed a bug which changed the rng behaviour: #1232. So it's not only about reproducibility but warning the users that the old way might be broken.
I am not sure if it should stay or not though. Seems like it belongs to the changelogs now.
…st fixture to use a seed value directly
… references to legacy_random
…and GMM clustering
2 participants
This pull request refactors and standardizes the use of random seeds for reproducibility across several modules, replacing ambiguous or inconsistent
random_stateparameters with a uniformseedparameter. It also introduces utility functions for managing random number generators, and updates the documentation and validation logic accordingly.These changes improve reproducibility, consistency, and clarity in the codebase's handling of random seeds and random number generation.