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.

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

[MRG] _get_estimators_indices() in BaseBagging class does not generate indices properly #16437

Merged
merged 14 commits into from Feb 20, 2020
Merged

[MRG] _get_estimators_indices() in BaseBagging class does not generate indices properly #16437

merged 14 commits into from Feb 20, 2020

Conversation

chofchof
Copy link
Contributor

Reference Issues/PRs

Fixes #16436.

What does this implement/fix? Explain your changes.

It solves the bug reported by the issue #16436, _get_estimators_indices() in BaseBagging class does not generate indices properly.

As Joel Nothman suggested, the seed is passed directly into _generate_bagging_indices() instead of the numpy RandomState object generated by the seed.

Any other comments?

None.

@jnothman
Copy link
Member

Thanks for the pull request!
Tests are failing. And you'll need to add a new one to test this issue.
Thanks for the pull request! Please make the title of the PR more descriptive. The title will become the commit message when this is merged. You should state what issue (or PR) it fixes/resolves in the description using the syntax described here.

glemaitre
glemaitre reviewed Feb 13, 2020
View reviewed changes
Copy link
Contributor

@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.

As a regression test, you can use the one that you reported in the issue. This is good.

sklearn/ensemble/_bagging.py Outdated Show resolved Hide resolved
sklearn/ensemble/_bagging.py Outdated Show resolved Hide resolved
@glemaitre
Copy link
Contributor

I am wondering if we have something similar regarding the feature indices?

@glemaitre
Copy link
Contributor

Regarding the failing tests, this is due to the change of the score in one of the docstring. This should be corrected as well.

@glemaitre glemaitre added this to REVIEWED AND WAITING FOR CHANGES in Guillaume's pet Feb 13, 2020
@chofchof chofchof changed the title Pull requests for Issues #16436 [WIP] _get_estimators_indices() in BaseBagging class does not generate indices properly Feb 14, 2020
@chofchof chofchof closed this Feb 14, 2020
@chofchof
Copy link
Contributor Author

I am wondering if we have something similar regarding the feature indices?

No similar problem regarding the feature_indices. The method _get_estimators_indices() is used only in the property estimators_samples_, but this property takes sample_indices only.

@chofchof chofchof reopened this Feb 14, 2020
@chofchof chofchof changed the title [WIP] _get_estimators_indices() in BaseBagging class does not generate indices properly [MRG] _get_estimators_indices() in BaseBagging class does not generate indices properly Feb 14, 2020
glemaitre
glemaitre reviewed Feb 14, 2020
View reviewed changes
Copy link
Contributor

@glemaitre glemaitre left a comment •
edited

Choose a reason for hiding this comment

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

Please add an entry to the change log at doc/whats_new/v0.23.rst under bug fixes. Like the other entries there, please reference this pull request with :pr: and credit yourself (and other contributors if applicable) with :user:

I think that we change the behaviour of ExtraTrees* as well, so we should add a note in the what's new on the top (change in behaviour section)

sklearn/ensemble/tests/test_bagging.py Show resolved Hide resolved
sklearn/ensemble/tests/test_bagging.py Outdated Show resolved Hide resolved
sklearn/ensemble/tests/test_bagging.py Show resolved Hide resolved
sklearn/ensemble/tests/test_bagging.py Outdated Show resolved Hide resolved
sklearn/ensemble/tests/test_bagging.py Outdated Show resolved Hide resolved
@chofchof
Copy link
Contributor Author

chofchof commented Feb 17, 2020 •
edited

Please add an entry to the change log at doc/whats_new/v0.23.rst under bug fixes. Like the other entries there, please reference this pull request with :pr: and credit yourself (and other contributors if applicable) with :user:

Added an entry to the change log as you suggested. Thanks a lot.
May I add your name as a contributor?

I think that we change the behaviour of ExtraTrees* as well, so we should add a note in the what's new on the top (change in behaviour section)

I have no idea on this one. Do I have to do something more?

glemaitre
glemaitre reviewed Feb 17, 2020
View reviewed changes
doc/whats_new/v0.23.rst Outdated Show resolved Hide resolved
@glemaitre
Copy link
Contributor

May I add your name as a contributor?

No need for this

glemaitre
glemaitre approved these changes Feb 17, 2020
View reviewed changes
Copy link
Contributor

@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.

Apart from correcting the what's new entry, everything looks good.
We will need a second review.

@jnothman Could you have a look at this one?

@glemaitre glemaitre moved this from REVIEWED AND WAITING FOR CHANGES to TO BE MERGED in Guillaume's pet Feb 17, 2020
jnothman
jnothman reviewed Feb 17, 2020
View reviewed changes
doc/whats_new/v0.23.rst Outdated Show resolved Hide resolved
sklearn/ensemble/tests/test_bagging.py Outdated Show resolved Hide resolved
@glemaitre
Copy link
Contributor

I push a quick reformatting of the what's new to follow the 80 characters limit.
@jnothman I think it looks good now.

rth
rth approved these changes Feb 20, 2020
View reviewed changes
Copy link
Member

@rth rth left a comment

Choose a reason for hiding this comment

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

Thanks @chofchof ! LGTM.

@rth rth merged commit baa4f07 into scikit-learn:master Feb 20, 2020
@glemaitre glemaitre moved this from TO BE MERGED to SPRINT - TO REVIEW in Guillaume's pet Feb 20, 2020
@glemaitre glemaitre moved this from SPRINT - TO REVIEW to MERGED in Guillaume's pet Feb 20, 2020
thomasjpfan pushed a commit to thomasjpfan/scikit-learn that referenced this pull request Feb 22, 2020
@chofchof @thomasjpfan
FIX _get_estimators_indices() in BaseBagging (scikit-learn#16437)
a5727cf
panpiort8 pushed a commit to panpiort8/scikit-learn that referenced this pull request Mar 3, 2020
@chofchof @panpiort
FIX _get_estimators_indices() in BaseBagging (scikit-learn#16437)
7724dae
gio8tisu pushed a commit to gio8tisu/scikit-learn that referenced this pull request May 15, 2020
@chofchof @gio8tisu
FIX _get_estimators_indices() in BaseBagging (scikit-learn#16437)
8d14bfa
@NicolasHug NicolasHug mentioned this pull request Apr 14, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rth rth rth approved these changes

@glemaitre glemaitre glemaitre approved these changes

@jnothman jnothman

Assignees
No one assigned
Labels
None yet
Projects
Guillaume's pet
MERGED
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

_get_estimators_indices() in BaseBagging class does not generate indices properly
4 participants
@chofchof @jnothman @glemaitre @rth