MAINT Parameters validation for sklearn.tree.export_graphviz #26034

Charlie-XIAO · 2023-03-31T06:03:54Z

Reference Issues/PRs

Towards #24862.

What does this implement/fix? Explain your changes.

Automatic parameter validation for sklearn.tree.export_graphviz

glemaitre

We should remove the code that checks for the precision parameter (l.445 - l.457):

        # validate
        if isinstance(precision, Integral):
            if precision < 0:
                raise ValueError(
                    "'precision' should be greater or equal to 0."
                    " Got {} instead.".format(precision)
                )
        else:
            raise ValueError(
                "'precision' should be an integer. Got {} instead.".format(
                    type(precision)
                )
            )

sklearn/tree/_export.py

sklearn/tree/tests/test_export.py

jeremiedbb

Thanks for the PR @Charlie-XIAO. Here are some comments

sklearn/tree/_export.py

…e-XIAO/scikit-learn into param-val-export_graphviz

Charlie-XIAO · 2023-04-07T14:33:17Z

Hi @jeremiedbb, I have made the changes you suggested. Would you mind taking a look?

…aphviz

sklearn/tree/_export.py

…aphviz

sklearn/tree/tests/test_export.py

Charlie-XIAO · 2023-04-27T15:04:21Z

A few questions here:

Should we change the validation of feature_names and class_names to {list, ndarray} instead of {array-like}? (as mentioned in FIX export_text and export_graphviz accepts feature and class names as array-like #26289 for export_text)
Should we make the validation of decision_tree in export_text to be "no_validation" as well? (maybe in another PR)

adrinjalali · 2023-04-27T15:34:06Z

Should we change the validation of feature_names and class_names to {list, ndarray} instead of {array-like}? (as mentioned in FIX export_text and export_graphviz accepts feature and class names as array-like #26289 for export_text)

probably better to actually accept array-like.

Should we make the validation of decision_tree in export_text to be "no_validation" as well? (maybe in another PR)

sounds good.

jeremiedbb · 2023-04-27T15:36:14Z

Should we make the validation of decision_tree in export_text to be "no_validation" as well? (maybe in another PR)

export_text is different because it actually checks that the tree is an instance of DecisionTreeClassifier

Charlie-XIAO · 2023-04-28T16:25:26Z

Will update validation of feature_names and class_names to "array-like" after #26289 is merged (in which we decided to call check_array on those).

Charlie-XIAO · 2023-05-16T03:10:20Z

#26289 has been merged so I think this PR is ready for review again. Can maintainers take a look? @glemaitre @adrinjalali @jeremiedbb Thank you very much.

thomasjpfan

LGTM

…learn#26034) Co-authored-by: Guillaume Lemaitre <g.lemaitre58@gmail.com>

Charlie-XIAO added 2 commits March 31, 2023 13:42

MAINT Parameters validation for tree.export_graphviz

461c798

added to param val test

13646dd

github-actions bot added the module:tree label Mar 31, 2023

fixed constraints, removed unnecessary tests

a23ec8e

glemaitre added No Changelog Needed Validation related to input validation labels Apr 1, 2023

glemaitre self-requested a review April 1, 2023 17:58

glemaitre reviewed Apr 1, 2023

View reviewed changes

sklearn/tree/_export.py Outdated Show resolved Hide resolved

sklearn/tree/_export.py Outdated Show resolved Hide resolved

sklearn/tree/_export.py Outdated Show resolved Hide resolved

Charlie-XIAO added 2 commits April 2, 2023 02:32

resolved conversations

95322f3

added test cases for array-like

655f289

Charlie-XIAO commented Apr 1, 2023

View reviewed changes

sklearn/tree/_export.py Show resolved Hide resolved

fixed failing test cases

ce6bdd3

glemaitre reviewed Apr 3, 2023

View reviewed changes

sklearn/tree/tests/test_export.py Outdated Show resolved Hide resolved

Charlie-XIAO and others added 3 commits April 3, 2023 20:42

improved tests

630ca86

Merge branch 'main' into param-val-export_graphviz

e1dc1cf

Merge branch 'main' into param-val-export_graphviz

cd15e9b

jeremiedbb reviewed Apr 3, 2023

View reviewed changes

sklearn/tree/_export.py Outdated Show resolved Hide resolved

sklearn/tree/_export.py Outdated Show resolved Hide resolved

sklearn/tree/_export.py Show resolved Hide resolved

Charlie-XIAO added 2 commits April 4, 2023 02:43

resolved conversations

7bc31c0

Merge branch 'param-val-export_graphviz' of https://github.com/Charli…

b5622ac

…e-XIAO/scikit-learn into param-val-export_graphviz

Charlie-XIAO added 4 commits April 20, 2023 12:14

Merge remote-tracking branch 'upstream/main' into param-val-export_gr…

7fcecda

…aphviz

Merge remote-tracking branch 'upstream/main' into param-val-export_gr…

0527d8b

…aphviz

Merge remote-tracking branch 'upstream/main' into param-val-export_gr…

02eeb3f

…aphviz

Merge remote-tracking branch 'upstream/main' into param-val-export_gr…

69a25f4

…aphviz

Charlie-XIAO mentioned this pull request Apr 26, 2023

sklearn.tree.export_text failing when feature_names supplied #26265

Closed

adrinjalali reviewed Apr 27, 2023

View reviewed changes

sklearn/tree/_export.py Outdated Show resolved Hide resolved

Charlie-XIAO mentioned this pull request Apr 27, 2023

FIX export_text and export_graphviz accepts feature and class names as array-like #26289

Merged

Charlie-XIAO added 2 commits April 27, 2023 22:09

leave decision_tree as no validation

b5d1f26

Merge remote-tracking branch 'upstream/main' into param-val-export_gr…

db84ab9

…aphviz

adrinjalali reviewed Apr 27, 2023

View reviewed changes

sklearn/tree/tests/test_export.py Outdated Show resolved Hide resolved

reverted necessary test

cd75117

Charlie-XIAO added 2 commits May 9, 2023 19:01

Merge branch 'main' into param-val-export_graphviz

676462b

Merge branch 'main' into param-val-export_graphviz

df2bf00

adrinjalali approved these changes May 16, 2023

View reviewed changes

thomasjpfan approved these changes May 16, 2023

View reviewed changes

thomasjpfan merged commit be892f5 into scikit-learn:main May 16, 2023

Charlie-XIAO deleted the param-val-export_graphviz branch September 23, 2023 13:28

REDVM pushed a commit to REDVM/scikit-learn that referenced this pull request Nov 16, 2023

MAINT Parameters validation for sklearn.tree.export_graphviz (scikit-…

b101a2e

…learn#26034) Co-authored-by: Guillaume Lemaitre <g.lemaitre58@gmail.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

MAINT Parameters validation for sklearn.tree.export_graphviz #26034

MAINT Parameters validation for sklearn.tree.export_graphviz #26034

Charlie-XIAO commented Mar 31, 2023

glemaitre left a comment •

edited

Loading

jeremiedbb left a comment

Charlie-XIAO commented Apr 7, 2023

Charlie-XIAO commented Apr 27, 2023 •

edited

Loading

adrinjalali commented Apr 27, 2023

jeremiedbb commented Apr 27, 2023

Charlie-XIAO commented Apr 28, 2023

Charlie-XIAO commented May 16, 2023 •

edited

Loading

thomasjpfan left a comment

MAINT Parameters validation for sklearn.tree.export_graphviz #26034

MAINT Parameters validation for sklearn.tree.export_graphviz #26034

Conversation

Charlie-XIAO commented Mar 31, 2023

Reference Issues/PRs

What does this implement/fix? Explain your changes.

glemaitre left a comment • edited Loading

Choose a reason for hiding this comment

jeremiedbb left a comment

Choose a reason for hiding this comment

Charlie-XIAO commented Apr 7, 2023

Charlie-XIAO commented Apr 27, 2023 • edited Loading

adrinjalali commented Apr 27, 2023

jeremiedbb commented Apr 27, 2023

Charlie-XIAO commented Apr 28, 2023

Charlie-XIAO commented May 16, 2023 • edited Loading

thomasjpfan left a comment

Choose a reason for hiding this comment

glemaitre left a comment •

edited

Loading

Charlie-XIAO commented Apr 27, 2023 •

edited

Loading

Charlie-XIAO commented May 16, 2023 •

edited

Loading