-
-
Notifications
You must be signed in to change notification settings - Fork 26.2k
[MRG] Relaxed criterion specification so tree plot does not force calculate entropy for any DecisionTreeClassifier #13947
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
Conversation
… for any criterion in decision tree
…starting" This reverts commit 9a7ba92
…am when decision tree classifier computes entropy
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
I think we'd like to release this in 0.21.3, but it's not clear to me whether that release will be necessary, or whether we should just put this in 0.22 for now. I'd say, for now, we'll create the 0.21.3 section, because it's still very early to exclude needing another patch release.
Please merge master and add an entry to the 0.21.3 change log at doc/whats_new/v0.21.rst
. Like the other entries there, please reference this pull request with :pr:
and credit yourself (and other contributors if applicable) with :user:
.
Added scikit-learn#13947 to change log
@jnothman How do I merge master? I thought that someone with write access could do it. |
I meant to fetch the latest version of master, merge it into your branch,
commit and push. There are some instructions at
https://scikit-learn.org/dev/developers/contributing.html
|
Merged master and added to change log like you suggested. Is there anything else I need to follow up @jnothman or is it out of my hands now? Also, wasn't sure if I added to the change log correctly. Didn't see anybody else make pull requests to amend the change log. |
I was hoping you would amend the change log in this pull request (you can just merge your changs log branch into this one). The reason for merging master was only that I'd changed the change log. |
Update v0.21.rst
Done! Merged my other request into this one. |
Reverted 2 commits since I was developing for another issue on this branch. Moved those changes to a separate branch. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @fhoang7
Thanks! |
Reference Issues/PRs
Fixes #13944
What does this implement/fix? Explain your changes.
Removed hard coding of criterion in tree making function. Instead, passed in criterion as an additional input into the function and modified subsequent function calls accordingly. export.py test file was modified to ensure that all tests passed as a result of this change.
Any other comments?
Need feedback on my approach and potential next steps. I would assume that the next thing to do would be to write a separate test to ensure that the proper decision tree criteria will be printed when entropy is the desired metric.