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

Projects
None yet
3 participants
@chkoar
Copy link
Member

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)):

This comment has been minimized.

Copy link
@glemaitre

glemaitre Feb 15, 2019

Member

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

This comment has been minimized.

Copy link

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

This comment has been minimized.

Copy link
Member Author

commented Feb 15, 2019

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

@codecov

This comment has been minimized.

Copy link

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

This comment has been minimized.

Copy link
Member Author

commented Feb 23, 2019

@glemaitre early stopping issue

@chkoar

This comment has been minimized.

Copy link
Member Author

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

2 of 4 checks passed

LGTM analysis: Python Running analyses for revisions
Details
continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
LGTM analysis: JavaScript No code changes detected
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.