-
-
Notifications
You must be signed in to change notification settings - Fork 25.9k
[MRG+2] Fix for issue #6352 #6376
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
@@ -152,7 +152,7 @@ def get_color(value): | |||
# Classification tree | |||
color = list(colors['rgb'][np.argmax(value)]) | |||
sorted_values = sorted(value, reverse=True) | |||
alpha = int(np.round(255 * (sorted_values[0] - sorted_values[1]) / | |||
alpha = 0 if len(sorted_values) == 1 else int(np.round(255 * (sorted_values[0] - sorted_values[1]) / | |||
(1 - sorted_values[1]), 0)) |
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.
This is too long and not readable. Just use a regular multiline if / else instead of a ternary expression.
Other than the style comment, LGTM. |
Hi, @ogrisel. I think, everything is done. |
LGTM +1. (You can prefix your PR with [MRG+2]) |
out = StringIO() | ||
export_graphviz(clf, out_file=out, filled=True) | ||
contents1 = out.getvalue() | ||
contents2 = 'digraph Tree {\n' \ |
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.
Note from PEP8:
The preferred way of wrapping long lines is by using Python's implied line continuation inside parentheses, brackets and braces. Long lines can be broken over multiple lines by wrapping expressions in parentheses. These should be used in preference to using a backslash for line continuation.
For long strings you can do:
contents2 = ('digraph Tree {\n'
'node ...\n'
'0 [label ...]\n'
'}')
Alternatively in your particular case, this would work:
contents2 = '\n'.join(['digraph Tree{', 'node ...', '0 [label ...', '...'])
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've used this line separator because of consistency. All other lines in this file divided with backslash. I think in this case I should replace all backslashes.
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.
Fair enough
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.
So. Keep it with backslashes? Or apply PEP8 recommendation to all strings?
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.
Keep it with backslashes is fine, that's what I meant.
@TomDLT merge ? |
I hope you will merge before somebody pushes some changes to this files in master =) This little fix take a lot of time. |
This is a fix and tests for issue #6352, which allows to generate fill colors for degraded learning set.