8000 [MRG] Remove redundant logic in Decision Tree by nelson-liu · Pull Request #7340 · scikit-learn/scikit-learn · GitHub
[go: up one dir, main page]
Skip to content

[MRG] Remove redundant logic in Decision Tree #7340

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
Closed
wants to merge 2 commits into from
Closed

[MRG] Remove redundant logic in Decision Tree #7340

wants to merge 2 commits into from

Conversation

nelson-liu
Copy link
Contributor

Reference Issue

Fixes #7338

What does this implement/fix? Explain your changes.

Removes duplicate logic in tree building and edits min_samples_leaf to behave similarly to min_fraction_weight_leaf. Also removes automatic setting of min_samples_split from min_samples_split.

Any other comments?

A PR with the concrete changes proposed in #7338

@nelson-liu nelson-liu changed the title Remove tree redundant logic [MRG] Remove tree redundant logic Sep 4, 2016
@nelson-liu nelson-liu changed the title [MRG] Remove tree redundant logic [MRG] Remove redundant logic in Decision Tree Sep 4, 2016
@nelson-liu nelson-liu mentioned this pull request Sep 4, 2016
Closed
nelson-liu
nelson-liu reviewed Sep 4, 2016
View reviewed changes
sklearn/tree/tree.py
@@ -592,7 +591,8 @@ class DecisionTreeClassifier(BaseDecisionTree, ClassifierMixin):
Best nodes are defined as relative reduction in impurity.
If None then unlimited number of leaf nodes.

class_weight : dict, list of dicts, "balanced" or None, optional (default=None)
class_weight : dict, list of dicts, "balanced" or None, \
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This renders correctly, but is there a better way to do it?

@jnothman
Copy link
Member
jnothman commented Sep 5, 2016

I'm closing this PR, then...?

@jnothman jnothman closed this Sep 5, 2016
@jnothman
Copy link
Member
jnothman commented Sep 5, 2016

Or do you hope to remove that unused condition here?

@nelson-liu
Copy link
Contributor Author

I figure this might be a good place to do it.. I'm also fine with working in private a bit then opening a new PR.

By not working, you mean giving different results?

Yes, exactly.

@nelson-liu nelson-liu mentioned this pull request Sep 15, 2016
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Duplicate logic in Decision Tree Building
2 participants
@nelson-liu @jnothman
0