FIX export_text and export_graphviz accepts feature and class names as array-like
#26289
Conversation
There was a problem hiding this comment.
Thanks for the PR @Charlie-XIAO . This also needs a regression test. Otherwise I think LGTM.
|
@adrinjalali Tests added, but I'm not sure if the way I add them is neat enough. |
tree.export_text accepts feature and class names as array-liketree.export_text accepts feature and class names as ndarray
|
we could call check_array on the feature names and actually accept all array-like data. |
Calling from sklearn.utils.validation import check_array
a = ["first", "second"]
a_array = check_array(a, dtype=object, ensure_2d=False)
a_array[0] = "new_first"
print(a_array[0])
# new_first
print(a[0])
# firstI think |
In case of something like |
There are a few ways to interpret this. If you mean:
Logistically, I think most users will pass in |
|
@thomasjpfan @adrinjalali So now we are going for converting input to ndarray right? I also agree with that: this is consistent with what |
|
@Charlie-XIAO you can use |
I remembered that @thomasjpfan said this would make a copy of lists, which is not desirable? |
From #26289 (comment), I think it's okay and make the copy. |
|
@thomasjpfan How would the docstring be then? We cannot write "array-like" because array-like objects do not necessarily support |
If we convert inputs to ndarrays before using on them, then we can use "array-like". |
tree.export_text accepts feature and class names as ndarraytree.export_text and tree.export_graphviz accepts feature and class names as array-like
|
@thomasjpfan Changes made, please let me know if tests need to be improved or the way I use |
|
@adrinjalali @glemaitre Thanks for your review! I've decoupled the tests, please let me know if it is okay (it has some repeated code). Otherwise I can use your other suggestion (i.e., just run twice since they are fast). |
|
Oh right I completely forgot about parametrization lol, will do soon. |
|
Thanks @Charlie-XIAO LGTM. |
tree.export_text and tree.export_graphviz accepts feature and class names as array-likeexport_text and export_graphviz accepts feature and class names as array-like
… class names as array-like (scikit-learn#26289)
Reference Issues/PRs
Fixes #26265.
What does this implement/fix? Explain your changes.
Make
tree.export_textaccept both feature names and class names as numpy arrays. Before modification, bothtree.export_graphvizandtree.export_textaccept can deal with class names which are numpy arrays, but accept only lists during parameter validation. Moreover,tree.export_graphvizcan deal with feature names which are numpy arrays, buttree.export_textcannot, and both of them accept only lists during parameter validation.This fix makes both
export_textconsistent withexport_graphviz, i.e., being able to deal with feature names and class names which are array-like. I will change docstring and parameter validation forexport_graphvizin #26034.Any other comments?
Please let me know if I should add test cases for this.