opened
jnothman
username_0
export_graphviz currently has [code that checks](https://github.com/scikit-learn/scikit-learn/blob/4bba21659c68d65b4555ea86751aac1fab9fd8e4/sklearn/tree/export.py#L461) if it's operating on a Tree instance or an estimator with a `tree_` attribute, but also [has](https://github.com/scikit-learn/scikit-learn/blob/4bba21659c68d65b4555ea86751aac1fab9fd8e4/sklearn/tree/export.py#L392) [code](https://github.com/scikit-learn/scikit-learn/blob/4bba21659c68d65b4555ea86751aac1fab9fd8e4/sklearn/tree/export.py#L424) that forces it to only operate on an estimator. One or the other of these should change. Either we force it to be an estimator, or we test that it's okay to be a Tree!
export_graphviz should work with a sklearn.tree._tree.Tree
issue
created
mabelvj
username_1
Could I try this?
comment
created
mabelvj
username_1
Could I try this? Whenever they decide what's the best option.
comment
created
jnothman
username_0
I don't have a strong preference for either option. Neither will break
backwards compatibility and we could always add Tree support later.
currently all our ensembles are ensembles of tree classifiers/regressors, I
think, not of trees themselves. @username_2 might have an opinion as the
incumbent tree expert.
comment
created
glemaitre
username_2
I would say estimator. However, I need to go trough #1046 to understand why the code is currently as it is. I keep inform soon.
comment
created
glemaitre
username_2
`estimators_` in GradienBoosting are `DecisionTreeRegressor` and not `Tree` anymore. So IMO, there is needs to support anymore `Tree` directly. I would agree with @amueller comment mentioning that tree_.Tree is an internal thing and that we could keep the user away from it, now that this is possible.
comment
created
jnothman
username_0
_tree.Tree is not quite an internal thing, IMO. It's needed in some
applications, and it's accessible through a public attribute.

Still, I'm happy to not support it here.
comment
closed
jnothman
username_0
issue