Common tests: Check that random_state is not altered #1108

amueller · 2012-09-03T16:44:29Z

Not sure this is what we want, but I think it is:
fit should not change the random_state so that fitting twice on the same data set should produce the same result.
There was a comment by @ogrisel in #455 along these lines, I think.

The text was updated successfully, but these errors were encountered:

ogrisel · 2012-09-03T16:46:41Z

+1 but this will have a very wide impact.

amueller · 2012-09-03T16:49:01Z

Yeah, this is gonna be real fun ^^

ogrisel · 2012-09-03T16:51:37Z

We should probably change the current fit idiom:

self.random_state = check_random_state(self.random_state)

to:

self.random_state_ = random_state = check_random_state(self.random_state)

and use that one instead in fit. That would also require pass random_state=0 (fixed seed) in the constructors of all the randomized estimators. Some people might not like it...

amueller · 2012-09-03T16:54:54Z

Why do you want to make random_state_ an attribute? It only needs to be local to fit. If you need another random state somewhere else, why not generate it again from the self.random_state?

I'm -1 on fixing seeds by default.

GaelVaroquaux · 2012-09-03T17:00:20Z

+1 but this will have a very wide impact.

+1

GaelVaroquaux · 2012-09-03T17:01:46Z

Why do you want to make random_state_ an attribute? It only needs to be
local to fit. If you need another random state somewhere else, why not
generate it again from the self.random_state?

GMMs and HMMs can use it to sample observations from the model. People
expect that sampling twice will give them different observations.

ogrisel · 2012-09-03T17:03:45Z

I'm -1 on fixing seeds by default.

Indeed, this is not a requirement.

GMMs and HMMs can use it to sample observations from the model. People
expect that sampling twice will give them different observations.

I don't think so. The sampling method should have it's own random_state kwargs to make that controllable.

amueller · 2012-09-03T17:14:06Z

I agree with @ogrisel on the hmm issue.
About self.random_state_: I don't like this duplication of attributes for the sake of not changing the model.
I think it becomes pretty confusing to distinguish the two. I know we use this in some places and it can not always be avoided, but I think we should try.

ogrisel · 2012-09-03T17:17:37Z

If you want idem potent fit calls you need to remember the original seed somewhere. But this is actually not the random_state itself that should be stored, but an integer seed. I think we should write a use case for the API caller point of view before moving on with this discussion.

amueller · 2012-09-03T17:19:49Z

agreed :)

glouppe · 2012-09-04T06:38:55Z

To avoid confusion, we may rename random_state to seed in the constructors and then set random_state = check_random_state(self.seed) (or self.random_state_ = ... if it needs to be stored) in `fit? Too bad this would break the current API though... :(

mblondel · 2012-10-15T10:59:36Z

+1 for not overwriting self.random_state

PR #1200 had this problem too.

amueller · 2013-02-13T20:10:40Z

Closed by #1518 and #1670.

amueller mentioned this issue Sep 3, 2012

WIP: Estimator summary #804

Closed

amueller mentioned this issue Jan 5, 2013

MRG add common test to check if estimators overwrite their init params #1518

Merged

amueller closed this as completed Feb 13, 2013

amueller referenced this issue Feb 23, 2013

DOC developer guidelines for unit tests and classes_

68d378d

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Common tests: Check that random_state is not altered #1108

Common tests: Check that random_state is not altered #1108

Common tests: Check that random_state is not altered #1108

Common tests: Check that random_state is not altered #1108

Comments