Fixes #11128 : Default n_estimator value should be 100 by Olamyy · Pull Request #11172 · scikit-learn/scikit-learn

Olamyy · 2018-05-30T22:53:15Z

Fixes #11128

… 100

Olamyy · 2018-05-30T22:53:46Z

Upcoming commits with deprecation warning in progress.

jnothman

Please see our documentation on deprecating default values. Changing it like this would break backwards compatibility

jnothman

Please addd a test that the warning works correctly. We have helpers like assert_warns

jnothman · 2018-06-06T03:12:55Z

sklearn/ensemble/forest.py

        self.class_weight = class_weight

+        if self.n_estimators == 10:
+            warnings.warn("'n_estimators' default value was changed to 100 in version 0.20.", DeprecationWarning)


What we want is "n_estimators default value will change to 100 in version 0.20".

By convention, this warning should only be issued in fit, not __init__.

Fixed this in my latest commit.

jnothman · 2018-06-06T03:13:30Z

doc/whats_new/v0.20.rst

  several times in a row. :issue:`9234` by :user:`andrewww <andrewww>`
  and :user:`Minghui Liu <minghui-liu>`.

+- In both :class:`ensemble.RandomForestClassifier` and `ensemble.ExtraTreesClassifier`,


This should be under API changes, rather than enhancements.

Fixed this too in my latest commit.

Olamyy · 2018-06-06T10:39:58Z

@jnothman For the test, do I create a new, say, test_classifiers file and write the test function in there or how would you best advise this be done?

jnothman · 2018-06-06T10:52:34Z

add a function in sklearn/ensemble/tests/test_forest.py

jnothman · 2018-06-07T06:43:11Z

sklearn/ensemble/tests/test_forest.py

+
+@pytest.mark.parametrize('name', FOREST_ESTIMATORS)
+def test_n_estimator_raises_warning(name):
+    check_raises_n_estimator_warning(name)


There is no need to have a separate function.

Please also test that no warning is raised if the user explicitly passed a value

TomDLT · 2018-06-07T08:34:24Z

sklearn/ensemble/forest.py

        """
+        # Validate hyperparameters
+        if self.n_estimators == 10:
+            warnings.warn("'n_estimators' default value will change to 100 in version 0.20", DeprecationWarning)


We don't want to break the user code.
Currently, with your PR, a user using the default value will witness a change in its code, in particular increasing its fit time.

We probably want to just warn about future change:

we should not change the value to 100

the DeprecationWarning should be a FutureWarning

the warning needs to say "will be changed in 0.22", instead of "has changed in 0.20"

the warning could also be more pedagogical, explaining this value needs to be changed manually, as suggested in the issue Change default n_estimators in RandomForest (to 100?) #11128 (comment)

…passing

…20 to 0.22. Provided a simple helper on changing the default value to 100

jnothman · 2018-06-07T12:32:10Z

poldata.README.2.0

@@ -0,0 +1,118 @@
+
+=======


This doesn't belong here...

jnothman · 2018-06-07T12:32:55Z

sklearn/ensemble/forest.py

        """
+        # Validate hyperparameters
+        if self.n_estimators == 10:
+            warnings.warn("'n_estimators' default value will be changed to 100 in version 0.22. For optimal performance, you should make change this value to 100", FutureWarning)


I don't think that you should have the statement about optimal performance.

Also line length must be <80

jnothman · 2018-06-07T12:33:47Z

sklearn/ensemble/tests/test_forest.py

+    ForestClassifier = FOREST_CLASSIFIERS[name]
+    clf = ForestClassifier(n_estimators=100, random_state=1, max_features=1,
+                           max_depth=1)
+    assert_warns(DeprecationWarning, func=clf.fit, X=iris.data, y=iris.target)


shouldn't this be assert_no_warnings?

Totally missed that.

I've done well to fix it in my latest commit.

jnothman · 2018-06-07T12:34:50Z

sklearn/ensemble/forest.py

        self : object
        """
+        # Validate hyperparameters
+        if self.n_estimators == 10:


Often we would do something like change the default to None, and treat it as "warn and use 10" to avoid warning when the user explicitly sets n_estimators=10

Is this in the dev guide? I don't think so, right? Also that deprecations should be in fit is not, I think? New issue? ;)

@jnorthman, I'm not sure I understand you.
Are you advising I change the n_estimators value to None?

@amueller I've created two issues to take care of both of these.

Yes, defaulting n_estimators=None would be fine. n_estimators='warn'

Okay.
But I'm a bit confused now because @amueller suggested the default for n_estimators should still be 10
here.

Am I missing something or isn't that some sort of a conflict as you're suggesting it should be None.

qinhanmin2014 · 2018-06-08T02:45:53Z

I think we already have some good PRs to serve as examples (e.g., #9397 #10331 ) ? And I think @TomDLT 's comment(#11172 (comment)) has summarized what to do here.
I might be -1 to create new rules since current rules seems fine (though maybe not perfect) and multiple rules will introduce unnecessary difficulties to future work.
@Olamyy Please take some time to revert unnecessary changes, otherwise it will be really difficult to review your PR.

amueller · 2018-06-09T19:44:46Z

doc/tutorial/text_analytics/skeletons/exercise_01_language_train_model.py

 print(cm)

-#import matplotlib.pyplot as plt
+#import matlotlib.pyplot as plt


Please revert this unrelated change

amueller · 2018-06-09T19:45:48Z

sklearn/ensemble/forest.py

    Parameters
    ----------
-    n_estimators : integer, optional (default=10)
+    n_estimators : integer, optional (default=100)


The default is still 10. There should be a note that it'll change in 0.22

qinhanmin2014 · 2018-06-10T00:51:21Z

@Olamyy
(1) There are still unrelated changes in doc/tutorial, please carefully check the diff in the PR.
(2) I think we've reached consensus to follow the ways in #9379, #10331, #11172 (comment), #11172 (comment), so you can have a try unless @amueller disagree.
(3) I don;t like words like deprecate in the PR since we're not deprecating anything.

glemaitre · 2018-06-25T15:05:47Z

@Olamyy Are you still willing to fix this issue, otherwise I would take care about it.

Olamyy · 2018-06-25T18:25:00Z

Hi @glemaitre, yes I am. Been a little busy.

I should be able to complete it tonight.

glemaitre · 2018-06-25T18:37:22Z

#response_container_BBPPID{font-family: initial; font-size:initial; color: initial;} Great thanks ;) Sent from my phone - sorry to be brief and potential misspell.

…ent. Made sure all tests are passing.

jnothman · 2018-06-26T04:08:14Z

doc/tutorial/text_analytics/skeletons/exercise_01_language_train_model.py

 print(cm)

-#import matplotlib.pyplot as plt
+#import matlotlib.pyplot as plt


Please revert this unrelated change

jnothman · 2018-06-26T04:08:17Z

doc/tutorial/text_analytics/skeletons/exercise_02_sentiment.py

    # more useful.
    # Fit the pipeline on the training set using grid search for the parameters

-    # TASK: print the cross-validated scores for the each parameters set


Please revert this unrelated change

jnothman · 2018-06-26T04:09:14Z

sklearn/ensemble/tests/test_forest.py

+    ForestClassifier = FOREST_CLASSIFIERS[name]
+    clf = ForestClassifier(n_estimators=10, random_state=1, max_features=1,
+                           max_depth=1)
+    assert_warns(FutureWarning, func=clf.fit, X=iris.data, y=iris.target)


Explicitly passing n_estimators=10 should not result in a warning.
Passing nothing for n_estimators should result in n warning

jnothman · 2018-06-26T04:09:30Z

sklearn/ensemble/forest.py

        self : object
        """
+        # Validate hyperparameters
+        if self.n_estimators == 10:


Yes, defaulting n_estimators=None would be fine. n_estimators='warn'

sklearn-lgtm · 2018-06-26T04:16:16Z

This pull request introduces 1 alert when merging e8147fc into 8083ea4 - view on LGTM.com

new alerts:

1 for Unused import

Comment posted by LGTM.com

jnothman

The default value doesn't matter so much. The default behaviour must be to warn and use 10 estimators. And the behaviour when a number is passed must be to not warn.

amueller · 2018-07-12T20:52:04Z

sklearn/ensemble/tests/test_forest.py



+@pytest.mark.parametrize('name', FOREST_CLASSIFIERS)
+def test_warning_raised_with_deprecated_n_estimator(name):


should be called "no_warning_raised".

amueller · 2018-07-12T20:54:35Z

sklearn/ensemble/forest.py

        self : object
        """
+        # Validate hyperparameters
+        if not self.n_estimators:


should be "is None".

amueller · 2018-07-12T20:55:59Z

doc/whats_new/v0.20.rst

  :class:`linear_model.LogisticRegression` when ``verbose`` is set to 0.
  :issue:`10881` by :user:`Alexandre Sevin <AlexandreSev>`.

+- In both :class:`ensemble.RandomForestClassifier` and `ensemble.ExtraTreesClassifier`,


missing :class: for the extra trees. Also, the message should read something like " n_estimators will be changed to 100 in version 0.22. A FutureWarning is raised when the default value is used." or similar. n_estimators=10doesn't raise a warning (as is tested).

jnothman

We need to have the default n_estimators=None.

We would like to have this finished before an upcoming release, as we believe it is important that we correct old wrongs.

I hope it is okay if another contributor completes this work

qinhanmin2014 · 2018-07-20T09:14:11Z

Resolved in #11542, thanks @Olamyy for your work.

Olamyy added 2 commits May 30, 2018 23:50

Changed RandomForest and ExtraTreesClassifier default n_estimators to…

b6117a3

… 100

Changed RandomForest and ExtraTreesClassifier default n_estimators to…

7253a7c

… 100

jnothman reviewed May 31, 2018

View reviewed changes

Olamyy added 2 commits June 5, 2018 18:59

Updated depracation logic to follow the development guideline

ec68bf2

Updated depracation logic to follow the development guideline

1ec2b4d

jnothman reviewed Jun 6, 2018

View reviewed changes

Moved logic to fit and updated documentation to API Changes

ac2041b

Added test function to test depreaction warning

939ced3

jnothman reviewed Jun 7, 2018

View reviewed changes

TomDLT reviewed Jun 7, 2018

View reviewed changes

Olamyy added 3 commits June 7, 2018 12:03

Removed redundant test function and included test for correct values …

a4fcc2e

…passing

Changed DeprecationWarning to Future Warning. Updated version from 0.…

e584e55

…20 to 0.22. Provided a simple helper on changing the default value to 100

Changed DeprecationWarning to Future Warning. Updated version from 0.…

8b5cb95

…20 to 0.22. Provided a simple helper on changing the default value to 100

jnothman reviewed Jun 7, 2018

View reviewed changes

This was referenced Jun 7, 2018

DOC Instructions for changing default value of a certain parameter #11219

Closed

Add dev guide documentation for handling cases where users set explicit hyperparameter values for deprecated values. #11220

Closed

Changed assert_warns to assert_no_wars

3654c3e

Removed poldata.

22dc7bd

amueller reviewed Jun 9, 2018

View reviewed changes

Fixed assert_no_warnings call in test function

f90d675

Made requested changes: Reduced warning line to meet 80 char requirem…

8d96f42

…ent. Made sure all tests are passing.

Merge branch 'master' into n_estimator_should_be_100

e8147fc

jnothman reviewed Jun 26, 2018

View reviewed changes

Olamyy added 2 commits June 26, 2018 07:10

Reverted unintended commit back to original state

bdb2a3b

Removing unused imports

476baeb

jnothman reviewed Jun 26, 2018

View reviewed changes

Olamyy added 7 commits July 5, 2018 12:10

Implementing warn and use 10

6c3bac7

Implementing warn and use 10

4b63cb1

Implementing warn and use 10

ce2e20a

Implementing warn and use 10

92e3489

Rename test function to show intent

d616b33

Reduced line numbers to meet required 79 characters

f52c170

Cleaning trailing spaces.

a2b0d2d

amueller requested changes Jul 12, 2018

View reviewed changes

Olamyy added 2 commits July 13, 2018 14:47

Switching to is None from if not

cd10b03

Update function name to show correct intent

5953a72

amueller mentioned this pull request Jul 15, 2018

[MRG] EHN: Change default n_estimators to 100 for random forest #11542

Merged

jnothman reviewed Jul 15, 2018

View reviewed changes

qinhanmin2014 closed this Jul 20, 2018

Olamyy deleted the n_estimator_should_be_100 branch July 24, 2018 07:13



		@pytest.mark.parametrize('name', FOREST_CLASSIFIERS)
		def test_warning_raised_with_deprecated_n_estimator(name):

Uh oh!

Conversation

Olamyy commented May 30, 2018

Uh oh!

Olamyy commented May 30, 2018

Uh oh!

jnothman left a comment

Choose a reason for hiding this comment

Uh oh!

jnothman 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!

Olamyy commented Jun 6, 2018

Uh oh!

jnothman commented Jun 6, 2018

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!

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!

Olamyy Jun 26, 2018 • 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!

qinhanmin2014 commented Jun 8, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

amueller Jun 9, 2018 • 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!

qinhanmin2014 commented Jun 10, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

glemaitre commented Jun 25, 2018

Uh oh!

Olamyy commented Jun 25, 2018

Uh oh!

glemaitre commented Jun 25, 2018 via email

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!

sklearn-lgtm commented Jun 26, 2018

Uh oh!

jnothman left a comment

Olamyy Jun 26, 2018 •

edited

Loading

amueller Jun 9, 2018 •

edited

Loading

qinhanmin2014 commented Jun 10, 2018 •

edited

Loading