8000 FIX sklearn.tree: fix validation of class_names argument for plot_tree by 2maz · Pull Request #26903 · scikit-learn/scikit-learn · GitHub
[go: up one dir, main page]
Skip to content

FIX sklearn.tree: fix validation of class_names argument for plot_tree #26903

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
Merged
merged 6 commits into from
Jul 27, 2023
Merged

FIX sklearn.tree: fix validation of class_names argument for plot_tree #26903

merged 6 commits into from
Jul 27, 2023

Conversation

2maz
Copy link
Contributor
@2maz 2maz commented Jul 26, 2023

Reference Issues/PRs

What does this implement/fix? Explain your changes.

In sklearn.tree.plot_tree: allow documented use of 'class_names' arguments, i.e. setting class_names=True

Currently this raises:

 sklearn.utils._param_validation.InvalidParameterError: The 'class_names' parameter of plot_tree must be an instance of 'list' or None. Got True instead.

Replicate with:

from sklearn.datasets import load_iris
from sklearn import tree

clf = tree.DecisionTreeClassifier(random_state=0)
iris = load_iris()

clf = clf.fit(iris.data, iris.target)

# Plotting class_names=True will raise
tree.plot_tree(clf, class_names=True)

Any other comments?

Reproducible with the above code snippet

@2maz 2maz changed the title sklearn.tree: Fix validation of class_names argument for plot_tree FIX sklearn.tree: fix validation of class_names argument for plot_tree Jul 26, 2023
@github-actions
Copy link
github-actions bot commented Jul 26, 2023 •
edited
Loading

✔️ Linting Passed

All linting checks passed. Your pull request is in excellent shape! ☀️

Generated for commit: 6feb25f. Link to the linter CI: here

Micky774
Micky774 approved these changes Jul 26, 2023
View reviewed changes
Copy link
Contributor
@Micky774 Micky774 left a comment

Choose a reason for hiding this comment

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

Thank you for the PR @2maz! Nice simple change. Looks good, with one minor change in addition to what you've already done.

sklearn/tree/_export.py Outdated
@@ -79,7 +79,7 @@ def __repr__(self):
"decision_tree": [DecisionTreeClassifier, DecisionTreeRegressor],
"max_depth": [Interval(Integral, 0, None, closed="left"), None],
"feature_names": [list, None],
"class_names": [list, None],
"class_names": [list, None, "boolean"],
Copy link
Contributor

Choose a reason for hiding this comment

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

While we're here, we should probably also change list to "array-like" similarly to export_graphviz. Also, totally just a nit-pick, but I like keeping None as the final option by convention :)

Suggested change
"class_names": [list, None, "boolean"],
"class_names": ["array-like", "boolean", 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.

Thanks for the quick feedback. Just updated accordingly.

@Micky774 Micky774 added Quick Review For PRs that are quick to review Validation related to input validation Waiting for Second Reviewer First reviewer is done, need a second one! labels Jul 26, 2023
@Micky774
Copy link
Contributor

@2maz Just for future reference, please avoid force-pushing commits in these PRs. It's totally okay for now, but in general we pr 8000 efer to maintain the commit history within the branch/PR. When, eventually, we merge these into main we will squash all commits into one to keep the history of main clean, so there is no need to do so within this branch/PR.

It helps for keeping history, debugging, and following development. Let me know if you have any questions/concerns regarding this. Thanks :)

thomasjpfan
thomasjpfan reviewed Jul 26, 2023
View reviewed changes
Copy link
Member
@thomasjpfan thomasjpfan left a comment

Choose a reason for hiding this comment

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

Thank you for the PR @2maz !

@thomasjpfan thomasjpfan added this to the 1.3.1 milestone Jul 26, 2023
@thomasjpfan
Copy link
Member

I think this is a bug fix and should be in 1.3.1.

Please add an entry to the change log at doc/whats_new/v1.3.rst with tag |Fix|. Like the other entries there, please reference this pull request with :pr: and credit yourself (and other contributors if applicable) with :user:.

@thomasjpfan thomasjpfan merged commit b8d4f46 into scikit-learn:main Jul 27, 2023
punndcoder28 pushed a commit to punndcoder28/scikit-learn that referenced this pull request Jul 29, 2023
@2maz @thomasjpfan @punndcoder28
FIX fix validation of class_names argument for plot_tree (scikit-lear…
91e4bbd 
…n#26903)

Co-authored-by: Thomas J. Fan <thomasjpfan@gmail.com>
@adrinjalali adrinjalali mentioned this pull request Aug 18, 2023
Closed
glemaitre pushed a commit to glemaitre/scikit-learn that referenced this pull request Sep 18, 2023
@2maz @thomasjpfan @glemaitre
FIX fix validation of class_names argument for plot_tree (scikit-lear…
d1b9dca 
…n#26903)

Co-authored-by: Thomas J. Fan <thomasjpfan@gmail.com>
jeremiedbb pushed a commit that referenced this pull request Sep 20, 2023
@2maz @thomasjpfan @jeremiedbb
FIX fix validation of class_names argument for plot_tree (#26903)
349cdd9 
Co-authored-by: Thomas J. Fan <thomasjpfan@gmail.com>
REDVM pushed a commit to REDVM/scikit-learn that referenced this pull request Nov 16, 2023
@2maz @thomasjpfan @REDVM
FIX fix validation of class_names argument for plot_tree (scikit-lear…
418e7fd 
…n#26903)

Co-authored-by: Thomas J. Fan <thomasjpfan@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@thomasjpfan thomasjpfan thomasjpfan approved these changes

@Micky774 Micky774 Micky774 approved these changes

Assignees
No one assigned
Labels
Bug module:tree Quick Review For PRs that are quick to review Validation related to input validation Waiting for Second Reviewer First reviewer is done, need a second one!
Projects
None yet
Milestone
1.3.1
Development

Successfully merging this pull request may close these issues.
3 participants
@2maz @Micky774 @thomasjpfan
0