Skip to content

FIX Change MRO for some estimators #17934

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
Merged
merged 4 commits into from
Jul 17, 2020
Merged

FIX Change MRO for some estimators #17934

merged 4 commits into from
Jul 17, 2020

Conversation

alfaro96
Copy link
Member

@alfaro96 alfaro96 commented Jul 16, 2020 •
edited
Loading

Reference Issues/PRs

Continue #14884.
Required for #17806.

What does this implement/fix? Explain your changes.

This PR changes the MRO for sklearn.ensemble.HistGradientBoostingClassifier.

NicolasHug
NicolasHug approved these changes Jul 16, 2020
View reviewed changes
Copy link
Member

@NicolasHug NicolasHug left a comment

Choose a reason for hiding this comment

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

Thanks @alfaro96

@NicolasHug
Copy link
Member

Though I think it'd be fine to just fix those in #17806, provided that there are not too many

@alfaro96 alfaro96 changed the title FIX Change MRO for HistGradientBoostingClassifier FIX Change MRO for some estimators Jul 16, 2020
@alfaro96
Copy link
Member Author

alfaro96 commented Jul 16, 2020 •
edited
Loading

Though I think it'd be fine to just fix those in #17806, provided that there are not too many

I think that these are the last ones:

  • sklearn.model_selection.tests.test_search.MockClassifier
  • sklearn.model_selection.tests.test_validation.MockClassifier

I will modify these estimators and change the PR title and description.

Note: The tests will fail in #17806 regardless of this PR because of a unittest.mock.Mock estimator used in sklearn.metrics.tests.test_score_objects.test_multimetric_scorer_calls_method_once.

@alfaro96 alfaro96 changed the title FIX Change MRO for some estimators FIX Add inheritance and change MRO for some estimators Jul 16, 2020
@alfaro96 alfaro96 requested a review from NicolasHug July 16, 2020 14:39
rth
rth reviewed Jul 16, 2020
View reviewed changes
rth
rth reviewed Jul 16, 2020
View reviewed changes
@alfaro96 alfaro96 changed the title FIX Add inheritance and change MRO for some estimators FIX Change MRO for some estimators Jul 17, 2020
@alfaro96 alfaro96 requested a review from rth July 17, 2020 07:53
@alfaro96
Copy link
Member Author

Previous commit reverted. I will fix the remaining issues in #17806.

Thanks @rth and @NicolasHug for your patience!

@rth
Copy link
Member

rth commented Jul 17, 2020

Thanks @alfaro96 ! Merging as it's now a 2 line change.

@rth rth merged commit 0cfe98b into scikit-learn:master Jul 17, 2020
@alfaro96 alfaro96 deleted the change_mro_hgbc branch July 17, 2020 10:27
jayzed82 pushed a commit to jayzed82/scikit-learn that referenced this pull request Oct 22, 2020
@alfaro96 @jayzed82
FIX Change MRO for some estimators (scikit-learn#17934)
73c1496
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@NicolasHug NicolasHug Awaiting requested review from NicolasHug

@rth rth Awaiting requested review from rth

Assignees
No one assigned
Labels
module:ensemble
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@alfaro96 @NicolasHug @rth