Keep random_state as given in Sampling and KPCA - #753
Open
owgreen-dev wants to merge 1 commit into
Open
owgreen-dev wants to merge 1 commit into
owgreen-dev wants to merge 1 commit into
Conversation
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>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #750.
What this fixes
Sampling.__init__andKPCA.__init__store the resolved generator instead of the argument:Two consequences.
1.
get_params()returns a generator, not the seed.sklearn's contract is that
__init__stores arguments untouched andfitresolves them, soclone()/GridSearchCV/cross_val_scorecan rebuild the estimator fromget_params(). ARandomStatethere 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
Samplinginstance is not reproducible.Every
fitdraws its subset from the one generator created in__init__, which the previousfitalready advanced:KPCAhas the same construction: the generator is used for the subsample whensampling=True(kpca.py:291) and forwarded into the innerKernelPCA(kpca.py:337).Sampling(random_state=0).get_params()["random_state"]RandomState(...)0clone(Sampling(random_state=0)).random_stateRandomState(...)0Samplinginstance,fittwice, scores equalFalseTrueKPCA(sampling=True)instance,fittwice, scores equalFalseTrueThe change
The argument is stored as given.
fitcallscheck_random_state(self.random_state)into a local right before the subsample is drawn, so anint,None, or a caller-suppliedRandomStateall behave as they do in scikit-learn. The innerPyODKernelPCAalready 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_givenandtest_refit_is_deterministic(Sampling),test_random_state_stored_as_givenandtest_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.