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

[MRG] FIX use a stump as base estimator in RUSBoostClassifier #545

Merged

Conversation

chkoar
Copy link
Member

@chkoar chkoar commented Feb 15, 2019

What does this implement/fix? Explain your changes.

I don't think that the referenced paper mentions something about stumps but this change keep us in line with the original implementation of AdaBoostClassifier

@@ -151,7 +152,7 @@ def fit(self, X, y, sample_weight=None):
super(RUSBoostClassifier, self).fit(X, y, sample_weight)
return self

def _validate_estimator(self, default=DecisionTreeClassifier()):
def _validate_estimator(self, default=DecisionTreeClassifier(max_depth=1)):
Copy link
Member

Choose a reason for hiding this comment

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

I think that you are right. We should actually call the parent class then:
https://github.com/scikit-learn/scikit-learn/blob/7389dba/sklearn/ensemble/weight_boosting.py#L414

@pep8speaks
Copy link

pep8speaks commented Feb 15, 2019

Hello @chkoar! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2019-06-11 21:51:45 UTC

@chkoar
Copy link
Member Author

chkoar commented Feb 15, 2019

@glemaitre do you have any clue why this test is failing? Isn't it weird?

@codecov
Copy link

codecov bot commented Feb 23, 2019

Codecov Report

Merging #545 into master will decrease coverage by 0.05%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #545      +/-   ##
==========================================
- Coverage   98.88%   98.83%   -0.06%     
==========================================
  Files          84       82       -2     
  Lines        5184     5043     -141     
==========================================
- Hits         5126     4984     -142     
- Misses         58       59       +1
Impacted Files Coverage Δ
imblearn/ensemble/tests/test_weight_boosting.py 100% <100%> (ø) ⬆️
imblearn/ensemble/_weight_boosting.py 97.72% <100%> (+0.91%) ⬆️
imblearn/over_sampling/_smote.py 95.54% <0%> (-1.72%) ⬇️
imblearn/utils/estimator_checks.py 96.74% <0%> (-0.16%) ⬇️
imblearn/tests/test_pipeline.py 99% <0%> (-0.07%) ⬇️
imblearn/tests/test_common.py 97.43% <0%> (-0.07%) ⬇️
...n/under_sampling/_prototype_selection/_nearmiss.py 98.73% <0%> (-0.04%) ⬇️
...ling/_prototype_selection/_random_under_sampler.py 100% <0%> (ø) ⬆️
imblearn/utils/tests/test_docstring.py 76.92% <0%> (ø) ⬆️
...mpling/_prototype_generation/_cluster_centroids.py 100% <0%> (ø) ⬆️
... and 19 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f30d0cf...e7b4356. Read the comment docs.

@chkoar
Copy link
Member Author

chkoar commented Feb 23, 2019

@glemaitre early stopping issue

@chkoar
Copy link
Member Author

chkoar commented Feb 23, 2019

@glemaitre as you can see using a stump as the default estimator I believe that the performance ain't so good. So, what do you propose? To make a benchmark using stump and a full decision tree and decide then, make the stump default to be in line with scikit-learn or leave the full grown tree as default?

@glemaitre glemaitre changed the title Use a stump as base estimator in RUSBoostClassifier [MRG] FIX use a stump as base estimator in RUSBoostClassifier Jun 11, 2019
@glemaitre glemaitre merged commit e2fee9a into scikit-learn-contrib:master Jun 11, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants