Skip to content

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

Merged
thomasjpfan merged 6 commits intoscikit-learn:mainfrom
2maz:fix_plot_tree
Jul 27, 2023
Merged

FIX sklearn.tree: fix validation of class_names argument for plot_tree#26903
thomasjpfan merged 6 commits intoscikit-learn:mainfrom
2maz:fix_plot_tree

Conversation

@2maz
Copy link
Copy Markdown
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
Copy Markdown

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
Copy Markdown
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
"max_depth": [Interval(Integral, 0, None, closed="left"), None],
"feature_names": [list, None],
"class_names": [list, None],
"class_names": [list, None, "boolean"],
Copy link
Copy Markdown
Contributor

@Micky774 Micky774 Jul 26, 2023

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
Copy Markdown
Contributor Author

@2maz 2maz Jul 26, 2023

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
Copy Markdown
Contributor

Micky774 commented Jul 26, 2023

@2maz Just for future reference, please avoid force-pushing commits in these PRs. It's totally okay for now, but in general we prefer 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
Copy Markdown
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
Copy Markdown
Member

thomasjpfan commented Jul 26, 2023

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