Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

FIX RandomForestRegressor OOB fails with integer-values multiple targets #27817

Merged
merged 15 commits into from Nov 25, 2023
Merged

FIX RandomForestRegressor OOB fails with integer-values multiple targets #27817

merged 15 commits into from Nov 25, 2023

Conversation

danieleongari
Copy link
Contributor

@danieleongari danieleongari commented Nov 20, 2023 •
edited by glemaitre

Fixes #27814

Allow to compute the OOB score with RandomForestRegressor and an integral multi-target regression.

@github-actions GitHub Actions
Copy link

github-actions bot commented Nov 20, 2023 •
edited

✔️ Linting Passed

All linting checks passed. Your pull request is in excellent shape! ☀️

Generated for commit: 83a512e. Link to the linter CI: here

@danieleongari danieleongari changed the title fix issue #27814 Fix issue #27814: RandomForestRegressor OOB fails with integer-values multiple targets Nov 21, 2023
glemaitre
glemaitre reviewed Nov 22, 2023
View reviewed changes
doc/whats_new/v1.4.rst Outdated Show resolved Hide resolved
@glemaitre glemaitre changed the title Fix issue #27814: RandomForestRegressor OOB fails with integer-values multiple targets FIX RandomForestRegressor OOB fails with integer-values multiple targets Nov 22, 2023
@glemaitre
Copy link
Member

The failure reported was only some failure related to codecov failing to upload the coverage report.

@danieleongari
Copy link
Contributor Author

Thanks @glemaitre for the fast review: I addressed all your concerns.

@glemaitre glemaitre self-requested a review November 23, 2023 09:40
glemaitre
glemaitre reviewed Nov 23, 2023
View reviewed changes
Copy link
Member

@glemaitre glemaitre left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A couple of comments to improve the tests.

sklearn/ensemble/tests/test_forest.py Outdated Show resolved Hide resolved
sklearn/ensemble/tests/test_forest.py Outdated Show resolved Hide resolved
sklearn/ensemble/tests/test_forest.py Outdated Show resolved Hide resolved
sklearn/ensemble/tests/test_forest.py Outdated Show resolved Hide resolved
sklearn/ensemble/tests/test_forest.py Show resolved Hide resolved
sklearn/ensemble/tests/test_forest.py Outdated Show resolved Hide resolved
sklearn/ensemble/tests/test_forest.py Outdated Show resolved Hide resolved
sklearn/ensemble/tests/test_forest.py Outdated Show resolved Hide resolved
sklearn/ensemble/tests/test_forest.py Show resolved Hide resolved
@danieleongari
Copy link
Contributor Author

danieleongari commented Nov 23, 2023 •
edited

Hi @glemaitre,
I'm now computing from scratch the OOB prediction for nsamples_test=3 samples (not all, for sake of time) and comparing with estimator.oob_prediction_[:nsamples_test].

glemaitre
glemaitre approved these changes Nov 24, 2023
View reviewed changes
Copy link
Member

@glemaitre glemaitre left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just small changes for following the convention naming that we usually use. Otherwise, LGTM. Thanks @danieleongari

Adding a label to request a second review.

sklearn/ensemble/tests/test_forest.py Outdated Show resolved Hide resolved
sklearn/ensemble/tests/test_forest.py Outdated Show resolved Hide resolved
sklearn/ensemble/tests/test_forest.py Outdated Show resolved Hide resolved
sklearn/ensemble/tests/test_forest.py Outdated Show resolved Hide resolved
@glemaitre glemaitre added the Waiting for Second Reviewer First reviewer is done, need a second one! label Nov 24, 2023
@danieleongari
Copy link
Contributor Author

danieleongari commented Nov 24, 2023 •
edited

All suggestions by @glemaitre implemented in the last commit, thanks!

thomasjpfan
thomasjpfan reviewed Nov 24, 2023
View reviewed changes
Copy link
Member

@thomasjpfan thomasjpfan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the PR @danieleongari !

sklearn/ensemble/tests/test_forest.py Show resolved Hide resolved
sklearn/ensemble/tests/test_forest.py Show resolved Hide resolved
sklearn/ensemble/tests/test_forest.py Outdated Show resolved Hide resolved
sklearn/ensemble/tests/test_forest.py Outdated Show resolved Hide resolved
sklearn/ensemble/tests/test_forest.py Outdated Show resolved Hide resolved
doc/whats_new/v1.4.rst Outdated Show resolved Hide resolved
@danieleongari
Copy link
Contributor Author

thanks @glemaitre @thomasjpfan. All comments have been addressed.

thomasjpfan
thomasjpfan approved these changes Nov 25, 2023
View reviewed changes
Copy link
Member

@thomasjpfan thomasjpfan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@thomasjpfan thomasjpfan merged commit 94f0d6a into scikit-learn:main Nov 25, 2023
27 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@thomasjpfan thomasjpfan thomasjpfan approved these changes

@glemaitre glemaitre glemaitre approved these changes

Assignees
No one assigned
Labels
module:ensemble Waiting for Second Reviewer First reviewer is done, need a second one!
Projects
None yet
Milestone
No milestone
3 participants
@danieleongari @glemaitre @thomasjpfan