Repository: scikit-learn/scikit-learn
Pull request: [MRG] EHN: Change default n_estimators to 100 for random forest
<!--
Thanks for contributing a pull request! Please ensure you have taken a look at
the contribution guidelines: https://github.com/scikit-learn/scikit-learn/blob/master/CONTRIBUTING.md#pull-request-checklist
-->

#### Reference Issues/PRs
<!--
Example: Fixes #1234. See also #3456.
Please use keywords (e.g., Fixes) to create link to the issues or pull requests
you resolved, so that they will automatically be closed when your pull request
is merged. See https://github.com/blog/1506-closing-issues-via-pull-requests
-->
Fixes #11128.

#### What does this implement/fix? Explain your changes.
Issues deprecation warning message for the default n_estimators parameter for the forest classifiers. Test added for the warning message when the default parameter is used.

#### Any other comments?


<!--
Please be aware that we are a loose team of volunteers so patience is
necessary; assistance handling other issues is very welcome. We value
all user contributions, no matter how minor they are. If we are slow to
review, either the pull request needs some benchmarking, tinkering,
convincing, etc. or more likely the reviewers are simply busy. In either
case, we ask for your understanding during the review process.
For more information, see our FAQ on this topic:
http://scikit-learn.org/dev/faq.html#why-is-my-pull-request-not-getting-any-attention.

Thanks for contributing!
-->


Problem statement:
Change default n_estimators in RandomForest (to 100?)
Analysis of code on github shows that people use default parameters when they shouldn't. We can make that a little bit less bad by providing reasonable defaults. The default for n_estimators is not great imho and I think we should change it. I suggest 100.
We could probably run benchmarks with openml if we want to do something empirical, but I think anything is better than 10.

I'm not sure if I want to tag this 1.0 because really no-one should ever run a random forest with 10 trees imho and therefore deprecation of the current default will show people they have a bug.

Hints from issue discussion:
I would like to give it a shot. Is the default value 100 final? 
@ArihantJain456 I didn't tag this one as "help wanted" because I wanted to wait for other core devs to chime in before we do anything.
I agree. Bad defaults should be deprecated. The warning doesn't hurt.​

I'm also +1 for n_estimators=100 by default
Both for this and #11129 I would suggest that the deprecation warning includes a message to encourage the user to change the value manually.
Has someone taken this up? Can I jump on it?
Bagging* also have n_estimators=10. Is this also a bad default?
@jnothman I would think so, but I have less experience and it depends on the base estimator, I think? With trees, probably?
I am fine with changing the default to 100 for random forests and bagging. (with the usual FutureWarning cycle).

@jnothman I would rather reserve the Blocker label for things that are really harmful if not fixed. To me, fixing this issue is an easy nice-to-heave but does not justify delaying the release cycle.
Okay. But given the rate at which we release, I think it's worth making
sure that simple deprecations make it into the release.​ It's not a big
enough feature to *not* delay the release for it.

Commit message: Removed blank line

Patch to review:
diff --git a/sklearn/ensemble/forest.py b/sklearn/ensemble/forest.py
index b7a349d4b5a89..92552cef6b563 100644
--- a/sklearn/ensemble/forest.py
+++ b/sklearn/ensemble/forest.py
@@ -135,7 +135,7 @@ class BaseForest(six.with_metaclass(ABCMeta, BaseEnsemble)):
     @abstractmethod
     def __init__(self,
                  base_estimator,
-                 n_estimators=10,
+                 n_estimators='warn',
                  estimator_params=tuple(),
                  bootstrap=False,
                  oob_score=False,
@@ -242,6 +242,12 @@ def fit(self, X, y, sample_weight=None):
         -------
         self : object
         """
+
+        if self.n_estimators == 'warn':
+            warnings.warn("The default value of n_estimators will change from "
+                          "10 in version 0.20 to 100 in 0.22.", FutureWarning)
+            self.n_estimators = 10
+
         # Validate or convert input data
         X = check_array(X, accept_sparse="csc", dtype=DTYPE)
         y = check_array(y, accept_sparse='csc', ensure_2d=False, dtype=None)
@@ -399,7 +405,7 @@ class ForestClassifier(six.with_metaclass(ABCMeta, BaseForest,
     @abstractmethod
     def __init__(self,
                  base_estimator,
-                 n_estimators=10,
+                 n_estimators='warn',
                  estimator_params=tuple(),
                  bootstrap=False,
                  oob_score=False,
@@ -408,7 +414,6 @@ def __init__(self,
                  verbose=0,
                  warm_start=False,
                  class_weight=None):
-
         super(ForestClassifier, self).__init__(
             base_estimator,
             n_estimators=n_estimators,
@@ -638,7 +643,7 @@ class ForestRegressor(six.with_metaclass(ABCMeta, BaseForest, RegressorMixin)):
     @abstractmethod
     def __init__(self,
                  base_estimator,
-                 n_estimators=10,
+                 n_estimators='warn',
                  estimator_params=tuple(),
                  bootstrap=False,
                  oob_score=False,
@@ -758,6 +763,10 @@ class RandomForestClassifier(ForestClassifier):
     n_estimators : integer, optional (default=10)
         The number of trees in the forest.
 
+        .. deprecated:: 0.20
+            The default value of ``n_estimators`` will change from 10 in
+            version 0.20 to 100 in version 0.22.
+
     criterion : string, optional (default="gini")
         The function to measure the quality of a split. Supported criteria are
         "gini" for the Gini impurity and "entropy" for the information gain.
@@ -971,7 +980,7 @@ class labels (multi-output problem).
     DecisionTreeClassifier, ExtraTreesClassifier
     """
     def __init__(self,
-                 n_estimators=10,
+                 n_estimators='warn',
                  criterion="gini",
                  max_depth=None,
                  min_samples_split=2,
@@ -1032,6 +1041,10 @@ class RandomForestRegressor(ForestRegressor):
     n_estimators : integer, optional (default=10)
         The number of trees in the forest.
 
+       .. deprecated:: 0.20
+            The default value of ``n_estimators`` will change from 10 in
+            version 0.20 to 100 in version 0.22.
+
     criterion : string, optional (default="mse")
         The function to measure the quality of a split. Supported criteria
         are "mse" for the mean squared error, which is equal to variance
@@ -1211,7 +1224,7 @@ class RandomForestRegressor(ForestRegressor):
     DecisionTreeRegressor, ExtraTreesRegressor
     """
     def __init__(self,
-                 n_estimators=10,
+                 n_estimators='warn',
                  criterion="mse",
                  max_depth=None,
                  min_samples_split=2,
@@ -1268,6 +1281,10 @@ class ExtraTreesClassifier(ForestClassifier):
     n_estimators : integer, optional (default=10)
         The number of trees in the forest.
 
+       .. deprecated:: 0.20
+            The default value of ``n_estimators`` will change from 10 in
+            version 0.20 to 100 in version 0.22.
+
     criterion : string, optional (default="gini")
         The function to measure the quality of a split. Supported criteria are
         "gini" for the Gini impurity and "entropy" for the information gain.
@@ -1454,7 +1471,7 @@ class labels (multi-output problem).
         splits.
     """
     def __init__(self,
-                 n_estimators=10,
+                 n_estimators='warn',
                  criterion="gini",
                  max_depth=None,
                  min_samples_split=2,
@@ -1513,6 +1530,10 @@ class ExtraTreesRegressor(ForestRegressor):
     n_estimators : integer, optional (default=10)
         The number of trees in the forest.
 
+       .. deprecated:: 0.20
+            The default value of ``n_estimators`` will change from 10 in
+            version 0.20 to 100 in version 0.22.
+
     criterion : string, optional (default="mse")
         The function to measure the quality of a split. Supported criteria
         are "mse" for the mean squared error, which is equal to variance
@@ -1666,7 +1687,7 @@ class ExtraTreesRegressor(ForestRegressor):
     RandomForestRegressor: Ensemble regressor using trees with optimal splits.
     """
     def __init__(self,
-                 n_estimators=10,
+                 n_estimators='warn',
                  criterion="mse",
                  max_depth=None,
                  min_samples_split=2,
@@ -1728,6 +1749,10 @@ class RandomTreesEmbedding(BaseForest):
     n_estimators : integer, optional (default=10)
         Number of trees in the forest.
 
+       .. deprecated:: 0.20
+            The default value of ``n_estimators`` will change from 10 in
+            version 0.20 to 100 in version 0.22.
+
     max_depth : integer, optional (default=5)
         The maximum depth of each tree. If None, then nodes are expanded until
         all leaves are pure or until all leaves contain less than
@@ -1833,7 +1858,7 @@ class RandomTreesEmbedding(BaseForest):
     """
 
     def __init__(self,
-                 n_estimators=10,
+                 n_estimators='warn',
                  max_depth=5,
                  min_samples_split=2,
                  min_samples_leaf=1,
diff --git a/sklearn/ensemble/tests/test_forest.py b/sklearn/ensemble/tests/test_forest.py
index 0054707ba5a0d..70449f509a6a4 100644
--- a/sklearn/ensemble/tests/test_forest.py
+++ b/sklearn/ensemble/tests/test_forest.py
@@ -31,6 +31,7 @@
 from sklearn.utils.testing import assert_raises
 from sklearn.utils.testing import assert_warns
 from sklearn.utils.testing import assert_warns_message
+from sklearn.utils.testing import assert_no_warnings
 from sklearn.utils.testing import ignore_warnings
 
 from sklearn import datasets
@@ -1228,3 +1229,23 @@ def test_min_impurity_decrease():
             # Simply check if the parameter is passed on correctly. Tree tests
             # will suffice for the actual working of this param
             assert_equal(tree.min_impurity_decrease, 0.1)
+
+
+def test_nestimators_future_warning():
+    # Test that n_estimators future warning is raised. Will be removed in 0.22
+
+    all_estimators = [RandomForestClassifier, RandomForestRegressor,
+                      ExtraTreesClassifier, ExtraTreesRegressor,
+                      RandomTreesEmbedding]
+
+    # When n_estimators default value is used
+    msg_future = "The default value of n_estimators will change from "
+    "10 in version 0.20 to 100 in 0.22."
+
+    for Estimator in all_estimators:
+        est = Estimator()
+        est = assert_warns_message(FutureWarning, msg_future, est.fit, X, y)
+
+        # When n_estimators is a valid value not equal to the default
+        est = Estimator(n_estimators=100)
+        est = assert_no_warnings(est.fit, X, y)


Code review comments:
diff_hunk: @@ -758,6 +763,10 @@ class RandomForestClassifier(ForestClassifier):
     n_estimators : integer, optional (default=10)
         The number of trees in the forest.
 
+        .. deprecated:: 0.20
path: sklearn/ensemble/forest.py
text: @user1:
should be "versionchanged" not "deprecated"

@author:
versionchanged as one long word, no spaces?

@user1:
yes. ``git grep versionchanged``?

@author:
working on that.

diff_hunk: @@ -1228,3 +1229,23 @@ def test_min_impurity_decrease():
             # Simply check if the parameter is passed on correctly. Tree tests
             # will suffice for the actual working of this param
             assert_equal(tree.min_impurity_decrease, 0.1)
+
+
+def test_nestimators_future_warning():
path: sklearn/ensemble/tests/test_forest.py
text: @user1:
It might be better to you pytest.parametrize as above instead of the loop, which will run each estimator as a separate test.

@user3:
FYI:

```python
@user2.mark.parametrize('forest', [RandomForestClassifier(), RandomForestRegressor(),
                                    ExtraTreesClassifier(), ExtraTreesRegressor(),
                                    RandomTreesEmbedding()])
def test_n_estimators_future_warning(estimator):
    ....
    estimator.fit(X, y)
    ....
```

@user4:
Might be better to parametrize with classes,
```py
@user2.mark.parametrize('forest', [RandomForestClassifier, RandomForestRegressor,
                         [...]
```
then create the corresponding instances inside the test -- this works better for getting a human readable test name with pytest..

@user3:
Fair enough