ENH: Add private RNG normalizer - #14199
Conversation
|
I think we need to support legacy RandomState because scikit-learn still supports it (and actually does not support Generator IIRC), can you double check? |
636e95e to
cd46252
Compare
f907908 to
21efb67
Compare
scikit-learn accepts RandomState but not Generator instances (check_random_state, FastICA, KFold all reject generators), so the new rng parameter must pass legacy RandomState instances through unchanged. Simplifies the ICA infomax branch accordingly, which also removes the last np.random.RandomState spelling flagged by test_no_global_rng.
The _legacy_rng decorator now normalizes and injects the rng keyword itself, making the redundant body-level _check_rng_compat calls (and the helper) unnecessary at all 21 transition sites. ICA.__init__ keeps explicit handling so integer rng seeds stay intact for third-party random_state parameters during fitting.
The normalized-generator indirections duplicated every parameter list just to forward rng: drop _permutation_cluster_test_normalized (publics call the private implementation directly again, as before), un-split infomax back into a single decorated function, and derive seed_deprecated docdict from random_state_deprecated.
Drop per-domain assertions of decorator semantics that are covered once centrally in test_check (both-supplied TypeError, FutureWarning, plain int-vs-RandomState parity), and parametrize the spatio-temporal sibling functions. Domain-specific behavior keeps explicit pins: the per-event stream restart quirk, the ICA sklearn boundary, and nested wrapper delegation.
| @@ -366,6 +367,8 @@ def mixed_norm( | |||
| sure_alpha_grid="auto", | |||
| random_state=None, | |||
There was a problem hiding this comment.
Okay hopefully one last thing 🤞 For signatures like this, let's move the * to before the random-state. Some people might need to update their scripts to pass random_state= but probably not that many since this is so far down the list. And we've been trying to add * to callables anyway in reasonable places. Once that's done, random_state can be grouped with rng (and ideally placed immediately after it).
|
@bruAristimunha one last-last idea... we've talked about how our |
|
doing @larsoner :) |
|
Minimal CI is unhappy, you need some exception for |
|
(I'll cancel the others to save cycles) |
larsoner
left a comment
There was a problem hiding this comment.
Just had to fix a tiny bug with reST rendering, marking for merge-when-green, thanks in advance @bruAristimunha !

Closes #9233