Skip to content

Keep random_state as given in Sampling and KPCA - #753

Open
owgreen-dev wants to merge 1 commit into
yzhao062:masterfrom
owgreen-dev:fix/random-state-stored-as-given
Open

owgreen-dev wants to merge 1 commit into
yzhao062:masterfrom
owgreen-dev:fix/random-state-stored-as-given

Conversation

@owgreen-dev

Copy link
Copy Markdown

Fixes #750.

What this fixes

Sampling.__init__ and KPCA.__init__ store the resolved generator instead of the argument:

# pyod/models/sampling.py:108 and pyod/models/kpca.py:243
self.random_state = check_random_state(random_state)

Two consequences.

1. get_params() returns a generator, not the seed.

>>> Sampling(random_state=0).get_params()["random_state"]
RandomState(MT19937) at 0x...

sklearn's contract is that __init__ stores arguments untouched and fit resolves them, so clone()/GridSearchCV/cross_val_score can rebuild the estimator from get_params(). A RandomState there is not the configuration the user wrote, and clones made from it don't share a seed with the original in any reproducible way.

2. Refitting the same Sampling instance is not reproducible.

Every fit draws its subset from the one generator created in __init__, which the previous fit already advanced:

>>> s = Sampling(random_state=0)
>>> a = s.fit(X).decision_scores_
>>> b = s.fit(X).decision_scores_
>>> np.allclose(a, b)
False

KPCA has the same construction: the generator is used for the subsample when sampling=True (kpca.py:291) and forwarded into the inner KernelPCA (kpca.py:337).

before after
Sampling(random_state=0).get_params()["random_state"] RandomState(...) 0
clone(Sampling(random_state=0)).random_state fresh RandomState(...) 0
same Sampling instance, fit twice, scores equal False True
same KPCA(sampling=True) instance, fit twice, scores equal False True

The change

The argument is stored as given. fit calls check_random_state(self.random_state) into a local right before the subsample is drawn, so an int, None, or a caller-supplied RandomState all behave as they do in scikit-learn. The inner PyODKernelPCA already resolves the seed it receives in its own __init__, so it is unaffected. Default behaviour (random_state=None) is unchanged.

Tests added: test_random_state_stored_as_given and test_refit_is_deterministic (Sampling), test_random_state_stored_as_given and test_refit_with_sampling_is_deterministic (KPCA). pytest pyod/test/test_sampling.py pyod/test/test_kpca.py: 44 passed.

Same shape as #737 and #738.

Disclosure: found with a script that checks each detector for the sklearn parameter contract; change and tests written with Claude Code assistance and reviewed by hand.

Both constructors stored check_random_state(random_state) instead of the
argument, so get_params() returned a RandomState object and every fit
drew from the same generator; refitting a Sampling instance produced
different scores. Resolve the seed locally in fit instead, matching the
scikit-learn parameter contract.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Sampling and KPCA resolve random_state in __init__, so get_params() leaks a RandomState and a refit of Sampling is not reproducible

1 participant