FIX Fix RandomForestRegressor doesn't accept max_samples=1.0 #20156#20159

Merged

thomasjpfan merged 23 commits intoscikit-learn:mainfrom

murata-yu:rfr_bug_check_#20156

Jun 4, 2021

Contributor

murata-yu commented May 28, 2021 •

edited by thomasjpfan

Loading

Reference Issues/PRs

Fixes #20156

What does this implement/fix? Explain your changes.

fix max_samples range for (0, 1) to (0, 1]

Any other comments?

murata-yu added 3 commits

May 28, 2021 04:52


          Fix range error

2121e95


          Fix range (0, 1) to (0, 1]

1df5cd7


          Merge branch 'rfr_bug_check_#20156' of https://github.com/murata-yu/s…

e6a7e85

…cikit-learn into rfr_bug_check_#20156

github-actions bot added the module:ensemble label

ogrisel reviewed

View reviewed changes

Member

ogrisel left a comment

Thanks, this looks good. Could you please add a test to check that this edge is accepted? Test that the result of:

RandomForestRegressor(max_samples=1.0, random_state=0).fit(X_train, y_train).predict_proba(X_test)

is the same:

RandomForestRegressor(max_samples=None, random_state=0).fit(X_train, y_train).predict_proba(X_test)

ogrisel reviewed

View reviewed changes

sklearn/ensemble/_forest.py Outdated Show resolved Hide resolved

Member

jeremiedbb commented May 28, 2021

Thanks @murata-yu ! Please add a what's new entry.

murata-yu added 3 commits

May 31, 2021 00:21


          add test for max_samples boundary

3f8a2a6


          undo for not related to PR

722f084


          fix mse comparison

b190c07

Contributor Author

murata-yu commented May 31, 2021

Hi, there. I add tests on 3f8a2a6.

I have some questions to discuss.

FOREST_CLASSIFIERS have predict_proba, but FOREST_REGRESSORS don't have predict_proba. So I compared MSE of predict and truth. Is this OK?
I couldn't come up with a good name for the tests, so please let me know if you have a good idea.

murata-yu requested a review from ogrisel

May 31, 2021 01:42

Member

ogrisel commented May 31, 2021 •

edited

Loading

FOREST_CLASSIFIERS have predict_proba, but FOREST_REGRESSORS don't have predict_proba. So I compared MSE of predict and truth. Is this OK?

Yes!

I couldn't come up with a good name for the tests, so please let me know if you have a good idea.

I think the test names are fine.

ogrisel reviewed

View reviewed changes

Member

ogrisel left a comment

LGTM. Just a small comment. Also please document the change in doc/whats_new/v1.0.rst.

sklearn/ensemble/tests/test_forest.py Outdated Show resolved Hide resolved

sklearn/ensemble/tests/test_forest.py Outdated Show resolved Hide resolved

murata-yu added 2 commits

June 1, 2021 00:01


          Easy to read

3d0005f


          add whats_new

7a15e41

Contributor Author

murata-yu commented Jun 1, 2021

I documented the change in doc/whats_new/v1.0.rst on 7a15e41.

murata-yu requested a review from ogrisel

June 1, 2021 05:11

cmarmo added Waiting for Reviewer and removed Waiting for Reviewer labels

cmarmo reviewed

View reviewed changes

Contributor

cmarmo left a comment

Hi @murata-yu , sphinx is complaining about the syntax in the what's new file. This will fix the warning. Thanks for your work so far.

doc/whats_new/v1.0.rst Outdated Show resolved Hide resolved


          fix syntax err in what's new

b803bf7

cmarmo added the Waiting for Reviewer label

ogrisel reviewed

View reviewed changes

sklearn/ensemble/tests/test_forest.py Outdated Show resolved Hide resolved


          Update sklearn/ensemble/tests/test_forest.py

80959ea

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

ogrisel reviewed

View reviewed changes

sklearn/ensemble/tests/test_forest.py Outdated Show resolved Hide resolved


          Update sklearn/ensemble/tests/test_forest.py

45d3af5

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

ogrisel reviewed

View reviewed changes

doc/whats_new/v1.0.rst Outdated Show resolved Hide resolved

ogrisel approved these changes

View reviewed changes

Member

ogrisel left a comment

Besides the above comment, LGTM. Thank you very much for the PR!


          fix API name

2ce6f58


          Merge branch 'rfr_bug_check_#20156' of https://github.com/murata-yu/s…

02df31f

…cikit-learn into rfr_bug_check_#20156

murata-yu requested a review from ogrisel

June 1, 2021 10:21

ogrisel reviewed

View reviewed changes

sklearn/ensemble/tests/test_forest.py Outdated Show resolved Hide resolved


          Update sklearn/ensemble/tests/test_forest.py

thomasjpfan reviewed

View reviewed changes

Member

thomasjpfan left a comment

Thank you for the PR @murata-yu !

sklearn/ensemble/_forest.py Outdated Show resolved Hide resolved

sklearn/ensemble/_forest.py Outdated Show resolved Hide resolved

sklearn/ensemble/_forest.py Outdated Show resolved Hide resolved

sklearn/ensemble/_forest.py Outdated Show resolved Hide resolved

sklearn/ensemble/tests/test_forest.py Outdated Show resolved Hide resolved

sklearn/ensemble/_forest.py Outdated Show resolved Hide resolved

sklearn/ensemble/_forest.py Outdated Show resolved Hide resolved

sklearn/ensemble/tests/test_forest.py Outdated Show resolved Hide resolved

murata-yu and others added 8 commits

June 3, 2021 12:18


          Update sklearn/ensemble/_forest.py

8356a49

Co-authored-by: Thomas J. Fan <thomasjpfan@gmail.com>


          Update sklearn/ensemble/_forest.py

8ef092a

Co-authored-by: Thomas J. Fan <thomasjpfan@gmail.com>


          Update sklearn/ensemble/_forest.py

3e48590

Co-authored-by: Thomas J. Fan <thomasjpfan@gmail.com>


          Update sklearn/ensemble/_forest.py

2af61b0

Co-authored-by: Thomas J. Fan <thomasjpfan@gmail.com>


          Update sklearn/ensemble/_forest.py

742c240

Co-authored-by: Thomas J. Fan <thomasjpfan@gmail.com>


          fix range value to float

5b46672


          change train data to X_large and y_large

7f66d0c


          Update sklearn/ensemble/_forest.py

d5113e9

Co-authored-by: Thomas J. Fan <thomasjpfan@gmail.com>

murata-yu requested a review from thomasjpfan

June 3, 2021 04:28

thomasjpfan approved these changes

View reviewed changes

Member

thomasjpfan left a comment

Thank you for the updates @murata-yu !

I left a small comment regarding the whats new. Otherwise this PR looks good to me.

doc/whats_new/v1.0.rst Outdated Show resolved Hide resolved

thomasjpfan changed the title ~~[MRG] Fix RandomForestRegressor doesn't accept max_samples=1.0 #20156~~ FIX Fix RandomForestRegressor doesn't accept max_samples=1.0 #20156


          Update doc/whats_new/v1.0.rst

928e29e

Co-authored-by: Thomas J. Fan <thomasjpfan@gmail.com>

thomasjpfan merged commit a1a6b3a into scikit-learn:main

thomasjpfan added a commit to thomasjpfan/scikit-learn that referenced this pull request


          DOC Fix GitHub regex labeler (#75)

b87304d

* TST enable test docstring params for feature extraction module (scikit-learn#20188)

* DOC fix a reference in sklearn.ensemble.GradientBoostingRegressor (scikit-learn#20198)

* FIX mcc zero divsion  (scikit-learn#19977)

* TST Add TransformedTargetRegressor to test_meta_estimators_delegate_data_validation (scikit-learn#20175)

Co-authored-by: Guillaume Lemaitre <g.lemaitre58@gmail.com>

* TST enable n_feature_in_ test for feature_extraction module

* FIX Uses points instead of pixels in plot_tree (scikit-learn#20023)

* MNT n_features_in through the multiclass module (scikit-learn#20193)

* CI Removes python 3.6 builds from wheel building (scikit-learn#20184)

* FIX Fix typo in error message in `fetch_openml` (scikit-learn#20201)

* FIX Fix error when using Calibrated with Voting (scikit-learn#20087)

* FIX Fix RandomForestRegressor doesn't accept max_samples=1.0 (scikit-learn#20159)

Co-authored-by: Olivier Grisel <olivier.grisel@ensta.org>
Co-authored-by: Thomas J. Fan <thomasjpfan@gmail.com>

* ENH Adds Poisson criterion in RandomForestRegressor (scikit-learn#19836)

Co-authored-by: Christian Lorentzen <lorentzen.ch@gmail.com>
Co-authored-by: Alihan Zihna <alihanz@gmail.com>
Co-authored-by: Alihan Zihna <a.zihna@ckhgbdp.onmicrosoft.com>
Co-authored-by: Chiara Marmo <cmarmo@users.noreply.github.com>
Co-authored-by: Olivier Grisel <olivier.grisel@gmail.com>
Co-authored-by: naozin555 <37050583+naozin555@users.noreply.github.com>
Co-authored-by: Venkatachalam N <venky.yuvy@gmail.com>
Co-authored-by: Thomas J. Fan <thomasjpfan@gmail.com>

* TST Replace assert_warns from decomposition/tests (scikit-learn#20214)

* TST check n_features_in_ in pipeline module (scikit-learn#20192)

Co-authored-by: Olivier Grisel <olivier.grisel@ensta.org>
Co-authored-by: Jérémie du Boisberranger <34657725+jeremiedbb@users.noreply.github.com>
Co-authored-by: Olivier Grisel <olivier.grisel@gmail.com>

* Allow `n_knots=None` if knots are explicitly specified in `SplineTransformer` (scikit-learn#20191)


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

* FIX make check_complex_data deterministic (scikit-learn#20221)

* TST test_fit_docstring_attributes include properties (scikit-learn#20190)

* FIX Uses the color max for colormap in ConfusionMatrixDisplay (scikit-learn#19784)

* STY Changing .format method to f-string formatting (scikit-learn#20215)

* CI Adds permissions for label action

Co-authored-by: Jérémie du Boisberranger <34657725+jeremiedbb@users.noreply.github.com>
Co-authored-by: tsuga <2888173+tsuga@users.noreply.github.com>
Co-authored-by: Conner Shen <connershen98@hotmail.com>
Co-authored-by: Guillaume Lemaitre <g.lemaitre58@gmail.com>
Co-authored-by: mlondschien <61679398+mlondschien@users.noreply.github.com>
Co-authored-by: Clément Fauchereau <clement.fauchereau@ensta-bretagne.org>
Co-authored-by: murata-yu <67666318+murata-yu@users.noreply.github.com>
Co-authored-by: Olivier Grisel <olivier.grisel@ensta.org>
Co-authored-by: Brian Sun <52805678+bsun94@users.noreply.github.com>
Co-authored-by: Christian Lorentzen <lorentzen.ch@gmail.com>
Co-authored-by: Alihan Zihna <alihanz@gmail.com>
Co-authored-by: Alihan Zihna <a.zihna@ckhgbdp.onmicrosoft.com>
Co-authored-by: Chiara Marmo <cmarmo@users.noreply.github.com>
Co-authored-by: Olivier Grisel <olivier.grisel@gmail.com>
Co-authored-by: naozin555 <37050583+naozin555@users.noreply.github.com>
Co-authored-by: Venkatachalam N <venky.yuvy@gmail.com>
Co-authored-by: Nanshan Li <nanshanli@dsaid.gov.sg>
Co-authored-by: solosilence <abhishekkr23rs@gmail.com>

eddiebergman mentioned this pull request

Update scikit learn 1.2 automl/auto-sklearn#1611

Closed

54 tasks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

module:ensemble Waiting for Reviewer