8000 [MRG + 1] Set min_impurity_split in gradient boosting models by sebp · Pull Request #8007 · scikit-learn/scikit-learn · GitHub
[go: up one dir, main page]

Skip to content

[MRG + 1] Set min_impurity_split in gradient boosting models #8007

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

Merged
merged 1 commit into from
Dec 13, 2016

Conversation

sebp
Copy link
Contributor
@sebp sebp commented Dec 7, 2016

Reference Issue

Fixes #8006

What does this implement/fix? Explain your changes.

Pass self.min_impurity_split to DecisionTreeRegressor in BaseGradientBoosting._fit_stage.

@amueller
Copy link
Member
amueller commented Dec 7, 2016

Can you please add a test to check that it now works as expected?

8000

@sebp sebp force-pushed the fix_min_impurity_split branch from f1c355e to 50e30b2 Compare December 10, 2016 16:02
@sebp
Copy link
Contributor Author
sebp commented Dec 10, 2016

@amueller I added a test function to the PR

@@ -961,6 +961,18 @@ def test_max_leaf_nodes_max_depth():
assert_equal(tree.max_depth, 1)


def test_min_impurity_split():
# Test precedence of max_leaf_nodes over max_depth.
Copy link
Contributor

Choose a reason for hiding this comment

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

perhaps change this comment? Thanks for looking into this!

@sebp sebp force-pushed the fix_min_impurity_split branch from 50e30b2 to 5385982 Compare December 11, 2016 11:46
@sebp
Copy link
Contributor Author
sebp commented Dec 11, 2016

Good catch, I updated the comment.

@amueller
Copy link
Member

Can you please add an entry to whatsnew.rst to the bugfix section? Otherwise LGTM.

@amueller amueller changed the title [MRG] Set min_impurity_split in gradient boosting models [MRG + 1] Set min_impurity_split in gradient boosting models Dec 12, 2016
@sebp sebp force-pushed the fix_min_impurity_split branch from 5385982 to c901cc5 Compare December 13, 2016 18:35
@sebp
Copy link
Contributor Author
sebp commented Dec 13, 2016

Done, I added an entry to whats_new

self.min_impurity_split should be passed to
DecisionTreeRegressor in BaseGradientBoosting._fit_stage.

Fixes scikit-learn#8006
@sebp sebp force-pushed the fix_min_impurity_split branch from c901cc5 to b48fb08 Compare December 13, 2016 18:39
@jnothman
Copy link
Member

LGTM

@jnothman jnothman merged commit 3162f98 into scikit-learn:master Dec 13, 2016
sergeyf pushed a commit to sergeyf/scikit-learn that referenced this pull request Feb 28, 2017
@Przemo10 Przemo10 mentioned this pull request Mar 17, 2017
Sundrique pushed a commit to Sundrique/scikit-learn that referenced this pull request Jun 14, 2017
NelleV pushed a commit to NelleV/scikit-learn that referenced this pull request Aug 11, 2017
paulha pushed a commit to paulha/scikit-learn that referenced this pull request Aug 19, 2017
maskani-moh pushed a commit to maskani-moh/scikit-learn that referenced this pull request Nov 15, 2017
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.

Gradient boosting models ignore min_impurity_split parameter
4 participants
0