-
Notifications
You must be signed in to change notification settings - Fork 3.9k
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
Improved python tree plots #2304
Improved python tree plots #2304
Conversation
This reverts commit dd8bf14a3ba604b0dfae3b7bb1c64b6784d15e03.
… exception instead of doing nothing.
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.
@CharlesAuguste Totally agree with you! Plots look really amazing now! Thanks a lot for your contribution!
Only one remaining thing has some room for improvements, I think. In my opinion, legend can be somehow clarified. At least, some words are needed about that Increasing/Decreasing
is related to monotone constraints. Imagine, someone uses tree plot in their slides. To be honest, without any additional info (or code around a picture), it's not very clear, what means Increasing/Decreasing
...
@CharlesAuguste Nice, thanks! I think it looks clearer now. |
@henry0312 @chivee @guolinke @jameslamb Can you please help and give a second review to this PR? |
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.
I don't know enough C++ to approve the config.h
changes but looked over the rest and left a few minor stylistic comments. Otherwise, this is good from my perspective.
python-package/lightgbm/plotting.py
Outdated
if root['decision_type'] == '<=': | ||
l_dec, r_dec = '<=', '>' | ||
operator = "≤" |
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.
I think the code would be easier to understand if this was put in a named variable, e.g.
lte_symbol = "≤"
operator = lte_symbol
python-package/lightgbm/plotting.py
Outdated
elif info == 'internal_count': | ||
label += r'\n{0}: {1}'.format(info, root[info]) | ||
graph.node(name, label=label) | ||
l_dec, r_dec = 'yes', "no" |
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.
in my opinion, simple assignments like this should be on their own lines. Could you change this to?
l_dec = 'yes'
r_dec = 'no'
precision : int or None, optional (default=None) | ||
'split_gain', 'internal_value', 'internal_count', 'internal_weight', | ||
'leaf_count', 'leaf_weight', 'data_percentage'. | ||
precision : int or None, optional (default=3) |
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.
Could you expand this documentation please? What are the units and what is the implication of setting None
?
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.
@jameslamb This is nice catch, but I think this is beyond of this PR. precision
was introduced in #1424 and is used in several functions (e.g. in plot_importance()
(#1777)), so I'll address your comment in a separate PR after merging this one.
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.
makes sense!
python-package/lightgbm/plotting.py
Outdated
if 'monotone_constraints' in model: | ||
monotone_constraints = model['monotone_constraints'] | ||
else: | ||
monotone_constraints = None |
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.
could you please simplify this?
monotone_constraints = model.get('monotone_constraints', None)
Close-reopen to re-trigger Appveyor. |
@jameslamb Can you please review the changes made? |
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.
I took another look, looks good to me! This is an awesome improvement.
Great stuff, thanks guys 👍 (now we just need to figure out what to do with the other big PR to move it forward...) |
In this pull request, we tried to improve the way trees are plotted by the function
plot_tree
. Visually digestible trees are essential for interpreting, debugging, and improving tree-based methods. LightGBM's current implementation ofplot_tree
produces trees which are very hard to mentally process and which, in our opinion, lack some useful information, like which nodes are constrained and how much data flows down through each node.This PR makes several changes to the
plot_tree
function which increases the amount of information displayed in a given tree whilst, at the same time, making the tree more digestible.Moreover, we add monotonically constrained features to the model's meta information so that this information is accessible through the model.
The figures below show how the output from the current
plot_tree
function compares with the output from our proposedplot_tree
function, and, demonstrate how the proposed output contains significantly more information than the current output, yet, is much easier to interpret.Here is a list of the new features from this pull request:
Current LightGBM tree:
Tree generated by the code of this pull request