[MRG+2] FIX adaboost estimators not randomising correctly by jnothman · Pull Request #7411 · scikit-learn/scikit-learn

jnothman · 2016-09-13T10:23:27Z

Fixes #7408 , where adaboost estimators were given the same random state initialisation, resulting in poor performance.

At the same time, I realised the need for setting nested random_states. I think this should be a public utility. This PR also ensures nested random_state is set in ensembles and in common tests.

Caveats:

AdaBoost results will differ from previously, by necessity
Bagging results will differ from previously, unnecessarily, but reducing code duplication and fixing an issue of nested estimators. Objections welcome.
testing random_states (via set_random_state) will differ from previously
sklearn.utils.randomize_estimator is a bad name for the new utility to set nested random_state. Suggestions welcome!
sklearn.utils.testing.set_random_state has a better name, but is subtly and valuably different from the new function:
- it defaults to random_state=0 rather than system-wide random state
- it ignores warnings
CV splitters should support {get,set}_params() if they are to be affected by the new utility. (If and when they do get this support, it could change the states set for other parts of the estimator by the new utility, due to parameter iteration order.) PR welcome, IMO
other things with random_state but without {get,set}_params are not affected by the new utility; this is noted in its docstring.

amueller · 2016-09-13T19:12:54Z

hm looks good.
I'm a bit confused. I thought something similar recently happened in BaggingEstimator but I might have dreamt that...

amueller · 2016-09-13T19:14:49Z

sklearn/utils/__init__.py

+    random_state = check_random_state(random_state)
+    to_set = {}
+    for key in sorted(estimator.get_params(deep=True)):
+        if key == 'random_state' or key.endswith('_random_state'):


why do we want / need this?

Pipelines have random states too! This isn't necessarily the only way to do this, but having integer random_states in each component means that unit can be replicated.

There the application would be trying to make an estimator deterministic. That is not really related to the issue we're trying to fix here, is it?

The random ensembles currently initialise each estimator with an integer random state derived from their local random state. However, currently they only use the random_state param. They should be setting every random_state param.

hm... true from a reproducibility perspective. The question is a bit how much of this can / should we put on the user.... Or on the Pipeline? The contract could also be "setting random_state on an object makes it deterministic". Then it would be the Pipelines problem. But that might be too much magic? It would be a pretty clear contract, though.

hm... true from a reproducibility perspective. The question is a bit how much of this can / should we put on the user.... Or on the Pipeline? The contract could also be "setting random_state on an object makes it deterministic". Then it would be the Pipelines problem. But that might be too much magic? It would be a pretty clear contract, though.

Too much magic: it's impossible to enforce. Suppose that an object depends on something that we do not control, like a non-reproducible data source. I think that it is wise to only do simple things.

If an object depends on something that we don't control, then that object breaks the contract and it's not our fault ;)

Too much magic and highly impractical. It means every meta-estimator or wrapper needs a random_state param and needs to manage it to do just the same as this... Here we manage things like dict iteration order invariance; you're going to need a shared helper function anyway. We already define nested parameter management and the random_state convention. Let's use it.

That having been said, I think it might be a good idea for RandomizedSearchCV to (optionally) manage the random_state of scipy.stats RVs

That having been said, I think it might be a good idea for RandomizedSearchCV to (optionally) manage the random_state of scipy.stats RVs

That already happens.

Hm but shouldn't this only match __random_state if we want to match the sub-estimator random state?

I thought there would be no harm in allowing something to have multiple distinct parameters ending _random_state, but also including __random_state. No, it's not tested; yes, I can change it.

jnothman · 2016-09-14T04:05:28Z

@amueller and @GaelVaroquaux , let me know if you think I should make this more conservative, i.e. to only effect the ensembles.

jnothman · 2016-09-14T23:29:50Z

Pushing more conservative version. Adding what's new in case you want to include the fix in 0.18-final.

ogrisel

Besides the following minor comments, LGTM.

ogrisel · 2016-09-20T11:10:10Z

sklearn/ensemble/base.py

+    random_state = check_random_state(random_state)
+    to_set = {}
+    for key in sorted(estimator.get_params(deep=True)):
+        if key == 'random_state' or key.endswith('_random_state'):


Souldn't it be double _: key.endswith('__random_state') instead?

@amueller (I think) also questioned this. I wanted, but did not test, this to be applicable to the hypothetical case that an estimator had multiple random_states for multiple purposes. Is that silly?

I'll suppose I'll change it so it doesn't look wrong...

The multiple random_states case sounds like a YAGNI to me.

ogrisel · 2016-09-20T11:12:18Z

sklearn/ensemble/tests/test_base.py


    assert_true(isinstance(ensemble[0], Perceptron))
+    assert_equal(ensemble[0].random_state, None)
+    assert_true(isinstance(ensemble[1].random_state, int))


For completeness you could add:

assert_true(isinstance(ensemble[2].random_state, int))

ogrisel · 2016-09-20T11:53:29Z

sklearn/ensemble/tests/test_weight_boosting.py

            (alg, score)

+        # Check we used multiple estimators
+        assert_true(len(clf.estimators_) > 1)


Better user assert_greater(len(clf.estimators_), 1) to get more informative error messages (when using nosetests).

ogrisel · 2016-09-20T11:54:28Z

sklearn/ensemble/tests/test_weight_boosting.py

+        # Check we used multiple estimators
+        assert_true(len(clf.estimators_) > 1)
+        # Check for distinct random states (see issue #7408)
+        assert_true(len(set(est.random_state for est in clf.estimators_)) > 1)


assert_greater(len(set(est.random_state for est in clf.estimators_)), 1)

ogrisel · 2016-09-20T11:55:07Z

sklearn/ensemble/tests/test_weight_boosting.py

+    # Check we used multiple estimators
+    assert_true(len(reg.estimators_) > 1)
+    # Check for distinct random states (see issue #7408)
+    assert_true(len(set(est.random_state for est in reg.estimators_)) > 1)


assert_greater

jnothman · 2016-09-20T23:58:58Z

Changes made, thanks @ogrisel. Am I leaving the what's new in 0.18 or has that train passed?

(fixes scikit-learn#7408) FIX ensure nested random_state is set in ensembles

jnothman · 2016-09-22T12:55:12Z

rebasing and marking MRG+1 due to @ogrisel's LGTM.

amueller · 2016-09-22T14:11:07Z

sklearn/ensemble/tests/test_weight_boosting.py

+        # Check we used multiple estimators
+        assert_greater(len(clf.estimators_), 1)
+        # Check for distinct random states (see issue #7408)
+        assert_greater(len(set(est.random_state


shouldn't they be equal to len(clf.estimators_) (with high probability)?

amueller · 2016-09-22T14:11:23Z

sklearn/ensemble/tests/test_weight_boosting.py

+    # Check we used multiple estimators
+    assert_true(len(reg.estimators_) > 1)
+    # Check for distinct random states (see issue #7408)
+    assert_greater(len(set(est.random_state for est in reg.estimators_)), 1)


amueller · 2016-09-22T14:12:46Z

LGTM apart from minor nitpick in tests.

NelleV

LGTM

GaelVaroquaux · 2016-09-23T05:34:58Z

LGTM.

Waiting for a couple of comments by @amueller in the tests before merging (on the len of unique random_state). But everything is for me. +1 for merge

jnothman · 2016-09-23T06:59:48Z

Tests improved. Thanks for the reviews.

jnothman · 2016-09-23T07:00:25Z

Merging. @amueller, please backport.

jnothman · 2016-09-23T07:01:18Z

(Actually, now I'm wondering if I should have merged before CIs were green... I checked that those more specific tests passed locally. Please excuse my unjustified hurry.)

amueller · 2016-09-23T07:08:47Z

would have probably been better to wait, but if master fails we'll know ;)

amueller · 2016-09-23T07:09:05Z

I'll backport everything all at once, when we're ready to release, I think.

* FIX adaboost estimators not randomising correctly (fixes #7408) FIX ensure nested random_state is set in ensembles * DOC add what's new * Only affect *__random_state, not *_random_state for now * TST More informative assertions for ensemble tests * More specific testing of different random_states

…rn#7411) * FIX adaboost estimators not randomising correctly (fixes scikit-learn#7408) FIX ensure nested random_state is set in ensembles * DOC add what's new * Only affect *__random_state, not *_random_state for now * TST More informative assertions for ensemble tests * More specific testing of different random_states

* tag '0.18': (1286 commits) [MRG + 1] More versionadded everywhere! (scikit-learn#7403) minor doc fixes fix lbfgs rename (scikit-learn#7503) minor fixes to whatsnew fix scoring function table fix rebase messup DOC more what's new subdivision DOC Attempt to impose some order on What's New 0.18 no fixed width within bold REL changes for release in 0.18.X branch (scikit-learn#7414) [MRG+2] Timing and training score in GridSearchCV (scikit-learn#7325) DOC: Added Nested Cross Validation Example (scikit-learn#7111) Sync docstring and definition default argument in kneighbors (scikit-learn#7476) added contributors for 0.18, minor formatting fixes. Fix typo in whats_new.rst [MRG+2] FIX adaboost estimators not randomising correctly (scikit-learn#7411) Addressing issue scikit-learn#7468. (scikit-learn#7472) Reorganize README clean up deprecation warning stuff in common tests [MRG+1] Fix regression in silhouette_score for clusters of size 1 (scikit-learn#7438) ...

* releases: (1286 commits) [MRG + 1] More versionadded everywhere! (scikit-learn#7403) minor doc fixes fix lbfgs rename (scikit-learn#7503) minor fixes to whatsnew fix scoring function table fix rebase messup DOC more what's new subdivision DOC Attempt to impose some order on What's New 0.18 no fixed width within bold REL changes for release in 0.18.X branch (scikit-learn#7414) [MRG+2] Timing and training score in GridSearchCV (scikit-learn#7325) DOC: Added Nested Cross Validation Example (scikit-learn#7111) Sync docstring and definition default argument in kneighbors (scikit-learn#7476) added contributors for 0.18, minor formatting fixes. Fix typo in whats_new.rst [MRG+2] FIX adaboost estimators not randomising correctly (scikit-learn#7411) Addressing issue scikit-learn#7468. (scikit-learn#7472) Reorganize README clean up deprecation warning stuff in common tests [MRG+1] Fix regression in silhouette_score for clusters of size 1 (scikit-learn#7438) ...

* dfsg: (1286 commits) [MRG + 1] More versionadded everywhere! (scikit-learn#7403) minor doc fixes fix lbfgs rename (scikit-learn#7503) minor fixes to whatsnew fix scoring function table fix rebase messup DOC more what's new subdivision DOC Attempt to impose some order on What's New 0.18 no fixed width within bold REL changes for release in 0.18.X branch (scikit-learn#7414) [MRG+2] Timing and training score in GridSearchCV (scikit-learn#7325) DOC: Added Nested Cross Validation Example (scikit-learn#7111) Sync docstring and definition default argument in kneighbors (scikit-learn#7476) added contributors for 0.18, minor formatting fixes. Fix typo in whats_new.rst [MRG+2] FIX adaboost estimators not randomising correctly (scikit-learn#7411) Addressing issue scikit-learn#7468. (scikit-learn#7472) Reorganize README clean up deprecation warning stuff in common tests [MRG+1] Fix regression in silhouette_score for clusters of size 1 (scikit-learn#7438) ...

…rn#7411) * FIX adaboost estimators not randomising correctly (fixes scikit-learn#7408) FIX ensure nested random_state is set in ensembles * DOC add what's new * Only affect *__random_state, not *_random_state for now * TST More informative assertions for ensemble tests * More specific testing of different random_states

jnothman added Bug Enhancement Waiting for Reviewer labels Sep 13, 2016

jnothman force-pushed the adaboost_random branch from 0561679 to 8e9e0c1 Compare September 13, 2016 10:30

This was referenced Sep 13, 2016

Bug in AdaBoostRegressor with randomstate #7408

Closed

[MRG+1] make more explicit which checks are run #7317

Merged

amueller reviewed Sep 13, 2016
View reviewed changes

jnothman force-pushed the adaboost_random branch from 7d4bca5 to 5e275f6 Compare September 14, 2016 23:30

jnothman added this to the 0.18 milestone Sep 14, 2016

jnothman removed the Enhancement label Sep 14, 2016

ogrisel approved these changes Sep 20, 2016

View reviewed changes

jnothman force-pushed the adaboost_random branch from b3cc749 to 4751362 Compare September 21, 2016 00:01

jnothman added 4 commits September 22, 2016 22:54

FIX adaboost estimators not randomising correctly

54d788c

(fixes scikit-learn#7408) FIX ensure nested random_state is set in ensembles

DOC add what's new

32f66af

Only affect *__random_state, not *_random_state for now

838d37d

TST More informative assertions for ensemble tests

3a1ba65

jnothman force-pushed the adaboost_random branch from 4751362 to 3a1ba65 Compare September 22, 2016 12:55

jnothman changed the title ~~[MRG] FIX adaboost estimators not randomising correctly~~ [MRG+1] FIX adaboost estimators not randomising correctly Sep 22, 2016

amueller reviewed Sep 22, 2016

View reviewed changes

NelleV approved these changes Sep 22, 2016

View reviewed changes

NelleV changed the title ~~[MRG+1] FIX adaboost estimators not randomising correctly~~ [MRG+2] FIX adaboost estimators not randomising correctly Sep 22, 2016

More specific testing of different random_states

2928bf0

jnothman merged commit 32d1236 into scikit-learn:master Sep 23, 2016

Uh oh!

Conversation

jnothman commented Sep 13, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

amueller commented Sep 13, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

GaelVaroquaux Sep 13, 2016 via email

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jnothman Sep 13, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jnothman commented Sep 14, 2016

Uh oh!

jnothman commented Sep 14, 2016

Uh oh!

ogrisel left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jnothman commented Sep 20, 2016

Uh oh!

jnothman commented Sep 22, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

amueller commented Sep 22, 2016

Uh oh!

NelleV left a comment

Choose a reason for hiding this comment

Uh oh!

GaelVaroquaux commented Sep 23, 2016

Uh oh!

jnothman commented Sep 23, 2016

Uh oh!

jnothman commented Sep 23, 2016

Uh oh!

jnothman commented Sep 23, 2016

Uh oh!

amueller commented Sep 23, 2016

Uh oh!

amueller commented Sep 23, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

jnothman commented Sep 13, 2016 •

edited

Loading

jnothman Sep 13, 2016 •

edited

Loading