Repository navigation
fix(ica): use a local RNG for BWS scores - #14382
viranovskaya wants to merge 1 commit into
Conversation
| def test_bws_score_local_rng(): | ||
| """BWS scores must not consume NumPy's global random state.""" | ||
| rng = np.random.default_rng(0) | ||
| # Enough observations to require random, rather than exact, permutations. | ||
| data = rng.standard_normal((2, 20)) | ||
| target = rng.standard_normal(20) | ||
| global_rng = check_random_state(None) | ||
| state = global_rng.get_state() | ||
| try: | ||
| scores = get_score_funcs()["bws_test"](data, target) | ||
| assert_equal(global_rng.get_state(), state) | ||
| assert_equal(scores.shape, (2,)) | ||
| finally: | ||
| global_rng.set_state(state) |
There was a problem hiding this comment.
Do we need this? Does mne/tests/test_docstring_parameters::test_no_global_rng already check this for us?
There was a problem hiding this comment.
The new test does not duplicate test_no_global_rng: the existing check is static and cannot detect global RNG use inside SciPy.
I verified this experimentally: with the old helper, the existing test passes, but the new one fails—all 624 elements of the RNG state array change. With the fix, both tests pass. I think this makes the new regression test worth keeping.
|
@viranovskaya are you sure this addresses all calls we make into the SciPy API (such that it would make sense to close #14379)? For now I've edited the PR description to remove the |
|
@drammock I audited MNE’s SciPy calls, including import aliases and dynamically selected functions, and cross-checked them against the RNG transitions in SciPy PR #26277. I found no additional calls requiring migration beyond the The two The targeted RNG and Infomax tests passed on both SciPy 1.14.1 and 1.18.1, with 10 tests passing in each environment. I have not tested a full build incorporating the upcoming SciPy changes. I’ll leave |
|
Thanks for detailing your process @viranovskaya. It strikes me as odd that the |
Reference issue (if any)
What does this implement/fix?
Summary
Fix the BWS score function used by
ICA.score_sourcesso that permutation sampling uses a local random number generator instead of NumPy's global random state.Changes
scipy.stats.PermutationMethodtobws_test.np.random.default_rng()instance.rngand olderrandom_statePermutationMethodAPIs for SciPy compatibility.This also avoids upcoming SciPy RNG-related warnings while keeping the existing BWS scoring behavior unchanged.
Additional information
AI assistance: Codex audited the relevant SciPy calls, prepared the implementation and regression test, simplified the test, and ran checks. I personally reviewed and verified all changes before submission. This disclosure note was prepared with Codex.