Repository: scikit-learn/scikit-learn
Pull request: FIX Fix RandomForestRegressor doesn't accept max_samples=1.0 #20156
<!--
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/main/CONTRIBUTING.md
-->

#### 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 #20156

#### What does this implement/fix? Explain your changes.
fix `max_samples` range for (0, 1) to (0, 1]

#### 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:
RandomForestRegressor doesn't accept max_samples=1.0
#### Describe the bug
This example from the doc works:

```py
from sklearn.ensemble import RandomForestRegressor
from sklearn.datasets import make_regression
X, y = make_regression(n_features=4, n_informative=2,
                       random_state=0, shuffle=False)
regr = RandomForestRegressor(max_depth=2, random_state=0)
regr.fit(X, y)
print(regr.predict([[0, 0, 0, 0]]))
```

Just changing one line to this:

```py
regr = RandomForestRegressor(max_depth=2, random_state=0, max_samples=1.0)
```

doesn't work anymore:

```python-traceback
ValueError: `max_samples` must be in range (0, 1) but got value 1.0
```
I believe `max_samples=None` (the default) and max_samples=1.0 should behave the same.

#### Steps/Code to Reproduce
see above

#### Versions
```
System:
    python: 3.8.5 (default, Jan 27 2021, 15:41:15)  [GCC 9.3.0]
executable: /usr/bin/python3
   machine: Linux-5.8.0-53-generic-x86_64-with-glibc2.29

Python dependencies:
          pip: 20.0.2
   setuptools: 45.2.0
      sklearn: 0.24.2
        numpy: 1.17.4
        scipy: 1.6.3
       Cython: None
       pandas: None
   matplotlib: 3.4.2
       joblib: 1.0.1
threadpoolctl: 2.1.0
Built with OpenMP: True
```

Hints from issue discussion:
I am able to confirm the error in the following environment.
```
System:
    python: 3.8.10 (default, May 12 2021, 15:46:43)  [GCC 8.3.0]
executable: /usr/local/bin/python
   machine: Linux-5.4.0-72-generic-x86_64-with-glibc2.2.5

Python dependencies:
          pip: 21.1.2
   setuptools: 57.0.0
      sklearn: 1.0.dev0
        numpy: 1.20.3
        scipy: 1.6.3
       Cython: 0.29.23
       pandas: 1.2.4
   matplotlib: 3.4.2
       joblib: 1.0.1
threadpoolctl: 2.1.0

Built with OpenMP: True
```

The cause is here. https://github.com/scikit-learn/scikit-learn/blob/309f135c3284d7db6e23ca81a87948c7066a3949/sklearn/ensemble/_forest.py#L109

Can I send a PR?
Well spotted, please send a PR.

sorry,,,
error message is `max_samples must be in range (0, 1) but got value 1.0` .
And, mathematically the error message seems correct .
https://en.wikipedia.org/wiki/Interval_(mathematics)
(`range (0, 1)` does not include 1)

so, I think I don't have to fix this.
The problem is that max_samples=1.0 and max_samples=None should behave the same.
Yes, max_samples=1.0 and max_samples=None should behave the same .
But, max_samples is not expected to be greater equal than 1.0 . 
So, I think it is correct tha an error occured at max_samples=1.0 .
max_samples=0.0 should crash (you cannot train a model without training
samples), max_samples=1.0 should work just fine (you _can_ train a model
using all the available training samples)


On Fri, May 28, 2021 at 3:02 PM murata-yu ***@***.***> wrote:

> Yes, max_samples=1.0 and max_samples=None should behave the same .
> But, max_samples is not expected to be greater equal than 1.0 .
> So, I think it is correct tha an error occured at max_samples=1.0 .
>
> —
> You are receiving this because you authored the thread.
> Reply to this email directly, view it on GitHub
> <https://github.com/scikit-learn/scikit-learn/issues/20156#issuecomment-850159999>,
> or unsubscribe
> <https://github.com/notifications/unsubscribe-auth/AAFUFABWJVGAEGHFE7TXDNTTP4WWRANCNFSM45VVMK4Q>
> .
>

I think accepting `max_samples=1.0` to mean `max_samples == n_samples` might have been considered problematic because we also accept `max_samples=1` to mean only one sample and that could be confusing for users that would not have observed that the type (int or float) is important.

But we do accept both `RandomForestRegressor(max_features=1.0)` and `RandomForestRegressor(max_features=1)` and they do mean different things.

So for consistency, +1 for accepting `max_samples=1.0` to be interpreted as 100% of `n_samples` and changing the docstrings to mention the range `(0.0, 1.0]` instead.

Note that many other subclasses might be impacted by this change so several docstrings will have to update updated.
I think the related PR should be merged and this issue closed.

Commit message: Update sklearn/ensemble/tests/test_forest.py

Co-authored-by: Olivier Grisel <olivier.grisel@ensta.org>

Patch to review:
diff --git a/doc/whats_new/v1.0.rst b/doc/whats_new/v1.0.rst
index 29a4bce98ecb0..1d3b6569ed25a 100644
--- a/doc/whats_new/v1.0.rst
+++ b/doc/whats_new/v1.0.rst
@@ -257,6 +257,10 @@ Changelog
   :class:`ensemble.StackingClassifier` and :class:`ensemble.StackingRegressor`.
   :pr:`19564` by `Thomas Fan`_.
 
+- |Fix| Fixed the range of the argument max_samples to be (0.0, 1.0]
+  in :class:`ensemble.BaseForest` and the class that inherits it.
+  :pr:`20159` by :user:`murata-yu`.
+
 :mod:`sklearn.feature_extraction`
 .................................
 
diff --git a/sklearn/ensemble/_forest.py b/sklearn/ensemble/_forest.py
index 5a93acd0c0554..08e24393f7510 100644
--- a/sklearn/ensemble/_forest.py
+++ b/sklearn/ensemble/_forest.py
@@ -86,7 +86,7 @@ def _get_n_samples_bootstrap(n_samples, max_samples):
     max_samples : int or float
         The maximum number of samples to draw from the total available:
             - if float, this indicates a fraction of the total and should be
-              the interval `(0, 1)`;
+              the interval `(0, 1]`;
             - if int, this indicates the exact number of samples;
             - if None, this indicates the total number of samples.
 
@@ -105,8 +105,8 @@ def _get_n_samples_bootstrap(n_samples, max_samples):
         return max_samples
 
     if isinstance(max_samples, numbers.Real):
-        if not (0 < max_samples < 1):
-            msg = "`max_samples` must be in range (0, 1) but got value {}"
+        if not (0 < max_samples <= 1):
+            msg = "`max_samples` must be in range (0, 1] but got value {}"
             raise ValueError(msg.format(max_samples))
         return round(n_samples * max_samples)
 
@@ -1163,7 +1163,7 @@ class RandomForestClassifier(ForestClassifier):
         - If None (default), then draw `X.shape[0]` samples.
         - If int, then draw `max_samples` samples.
         - If float, then draw `max_samples * X.shape[0]` samples. Thus,
-          `max_samples` should be in the interval `(0, 1)`.
+          `max_samples` should be in the interval `(0, 1]`.
 
         .. versionadded:: 0.22
 
@@ -1473,7 +1473,7 @@ class RandomForestRegressor(ForestRegressor):
         - If None (default), then draw `X.shape[0]` samples.
         - If int, then draw `max_samples` samples.
         - If float, then draw `max_samples * X.shape[0]` samples. Thus,
-          `max_samples` should be in the interval `(0, 1)`.
+          `max_samples` should be in the interval `(0, 1]`.
 
         .. versionadded:: 0.22
 
@@ -1557,6 +1557,7 @@ class RandomForestRegressor(ForestRegressor):
     >>> print(regr.predict([[0, 0, 0, 0]]))
     [-8.32987858]
     """
+
     def __init__(self,
                  n_estimators=100, *,
                  criterion="squared_error",
@@ -1789,7 +1790,7 @@ class ExtraTreesClassifier(ForestClassifier):
         - If None (default), then draw `X.shape[0]` samples.
         - If int, then draw `max_samples` samples.
         - If float, then draw `max_samples * X.shape[0]` samples. Thus,
-          `max_samples` should be in the interval `(0, 1)`.
+          `max_samples` should be in the interval `(0, 1]`.
 
         .. versionadded:: 0.22
 
@@ -1873,6 +1874,7 @@ class labels (multi-output problem).
     >>> clf.predict([[0, 0, 0, 0]])
     array([1])
     """
+
     def __init__(self,
                  n_estimators=100, *,
                  criterion="gini",
@@ -2095,7 +2097,7 @@ class ExtraTreesRegressor(ForestRegressor):
         - If None (default), then draw `X.shape[0]` samples.
         - If int, then draw `max_samples` samples.
         - If float, then draw `max_samples * X.shape[0]` samples. Thus,
-          `max_samples` should be in the interval `(0, 1)`.
+          `max_samples` should be in the interval `(0, 1]`.
 
         .. versionadded:: 0.22
 
@@ -2168,6 +2170,7 @@ class ExtraTreesRegressor(ForestRegressor):
     >>> reg.score(X_test, y_test)
     0.2708...
     """
+
     def __init__(self,
                  n_estimators=100, *,
                  criterion="squared_error",
diff --git a/sklearn/ensemble/tests/test_forest.py b/sklearn/ensemble/tests/test_forest.py
index c74a1ca0c603e..5834b7e403fe6 100644
--- a/sklearn/ensemble/tests/test_forest.py
+++ b/sklearn/ensemble/tests/test_forest.py
@@ -50,6 +50,8 @@
 from sklearn.svm import LinearSVC
 from sklearn.utils.validation import check_random_state
 
+from sklearn.metrics import mean_squared_error
+
 from sklearn.tree._classes import SPARSE_SPLITTERS
 
 
@@ -1419,16 +1421,14 @@ def test_forest_degenerate_feature_importances():
     'max_samples, exc_type, exc_msg',
     [(int(1e9), ValueError,
       "`max_samples` must be in range 1 to 6 but got value 1000000000"),
-     (1.0, ValueError,
-      r"`max_samples` must be in range \(0, 1\) but got value 1.0"),
      (2.0, ValueError,
-      r"`max_samples` must be in range \(0, 1\) but got value 2.0"),
+      r"`max_samples` must be in range \(0, 1\] but got value 2.0"),
      (0.0, ValueError,
-      r"`max_samples` must be in range \(0, 1\) but got value 0.0"),
+      r"`max_samples` must be in range \(0, 1\] but got value 0.0"),
      (np.nan, ValueError,
-      r"`max_samples` must be in range \(0, 1\) but got value nan"),
+      r"`max_samples` must be in range \(0, 1\] but got value nan"),
      (np.inf, ValueError,
-      r"`max_samples` must be in range \(0, 1\) but got value inf"),
+      r"`max_samples` must be in range \(0, 1\] but got value inf"),
      ('str max_samples?!', TypeError,
       r"`max_samples` should be int or float, but got "
       r"type '\<class 'str'\>'"),
@@ -1443,6 +1443,40 @@ def test_max_samples_exceptions(name, max_samples, exc_type, exc_msg):
         est.fit(X, y)
 
 
+@pytest.mark.parametrize('name', FOREST_REGRESSORS)
+def test_max_samples_boundary_regressors(name):
+    X_train, X_test, y_train, y_test = train_test_split(
+        X_reg, y_reg, train_size=0.7, test_size=0.3, random_state=0)
+
+    ms_1_model = FOREST_REGRESSORS[name](max_samples=1.0, random_state=0)
+    ms_1_predict = ms_1_model.fit(X_train, y_train).predict(X_test)
+
+    ms_None_model = FOREST_REGRESSORS[name](max_samples=None, random_state=0)
+    ms_None_predict = ms_None_model.fit(X_train, y_train).predict(X_test)
+
+    ms_1_ms = mean_squared_error(ms_1_predict, y_test)
+    ms_None_ms = mean_squared_error(ms_None_predict, y_test)
+
+    assert ms_1_ms == pytest.approx(ms_None_ms)
+
+
+@pytest.mark.parametrize('name', FOREST_CLASSIFIERS)
+def test_max_samples_boundary_classifiers(name):
+    rng = np.random.RandomState(1)
+
+    X_train = rng.randn(10000, 2)
+    y_train = rng.randn(10000) > 0
+    X_test = rng.randn(1000, 2)
+
+    ms_1_model = FOREST_CLASSIFIERS[name](max_samples=1.0, random_state=0)
+    ms_1_proba = ms_1_model.fit(X_train, y_train).predict_proba(X_test)
+
+    ms_None_model = FOREST_CLASSIFIERS[name](max_samples=None, random_state=0)
+    ms_None_proba = ms_None_model.fit(X_train, y_train).predict_proba(X_test)
+
+    assert np.testing.assert_allclose(ms_1_proba, ms_None_proba)
+
+
 def test_forest_y_sparse():
     X = [[1, 2, 3]]
     y = csr_matrix([4, 5, 6])


Code review comments:
diff_hunk: @@ -257,6 +257,10 @@ Changelog
   :class:`ensemble.StackingClassifier` and :class:`ensemble.StackingRegressor`.
   :pr:`19564` by `Thomas Fan`_.
 
+- |Fix| Fixed the range of the argument max_samples to be (0.0, 1.0]
+  in :class:`ensemble.BaseForest` and the class that inherits it.
path: doc/whats_new/v1.0.rst
text: @user1:
`BaseForest` is not really part of the public API. Maybe we could just mention something like

 :class:`ensemble.RandomForestClassifier`,  :class:`ensemble.RandomForestRegressor` and all other random forest based estimators.

@author:
fixed it on https://github.com/scikit-learn/scikit-learn/pull/20159/commits/2ce6f58f946810c74f101de7ea741fb80e56ccc8