-
Notifications
You must be signed in to change notification settings - Fork 91
Update visualize_decision_tree to include feature names #1813
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.
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
Conversation
…yx/evalml into 1757_estimator_input_feature_names
Refactor to add _manage_woodwork() helper function.
Codecov Report
@@ Coverage Diff @@
## main #1813 +/- ##
=========================================
- Coverage 100.0% 100.0% -0.0%
=========================================
Files 252 252
Lines 20061 20052 -9
=========================================
- Hits 20053 20044 -9
Misses 8 8
Continue to review full report at Codecov.
|
def test_decision_tree_data_from_estimator(fitted_tree_estimators): | ||
est_class, est_reg = fitted_tree_estimators | ||
|
||
formatted_ = decision_tree_data_from_estimator(est_reg, feature_names=[f'Testing_{col_}' for col_ in range(est_reg._component_obj.n_features_)]) | ||
formatted_ = decision_tree_data_from_estimator(est_reg) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removing feature names parameter. Since this test already checks that the output has the feature names but now we're just grabbing them from the estimator rather than setting explicitly, no other code changes needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Only comment I have is maybe we can make the feature names stand out a bit (bold or italicize it?).
@jeremyliweishih That's a great suggestion! With our current implementation though, I'm not sure there's an API available to easily do this though so we might need to punt on that: https://scikit-learn.org/stable/modules/generated/sklearn.tree.export_graphviz.html#sklearn.tree.export_graphviz |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good!
Closes #1718. Also removes
feature_names
as a parameter fromdecision_tree_data_from_estimator
, since we can extract the feature names used to fit from the estimator now.Not sure what the best way to add testing to our codebase to ensure the output png has the right name, but here's a code snippet and the resulting image 😁