bug fix on color handling for showProjection - #1070
Conversation
3da94bb to
133a7cc
Compare
SHZ66
left a comment
SHZ66
left a comment
There was a problem hiding this comment.
Import checker from ~matplotlib.pyplot.scatter or make it clearer in the documentation about the input format
eda4865 to
f1f296f
Compare
| raise ValueError('each element of colors should be a number or satisfy matplotlib color rules') | ||
|
|
||
| if not isinstance(color, type(colors[0])): | ||
| raise TypeError('each element of colors should have the same type') |
There was a problem hiding this comment.
Is this check necessary? Couldn't the matplotlib function handle colors defined in different ways in the same list?
There was a problem hiding this comment.
oh, I guess it probably could
|
|
||
| colors_dict = {} | ||
|
|
||
| if is_color_like(colors) or colors is None or isinstance(colors, Number): |
There was a problem hiding this comment.
Should colors is None be part of this? Wouldn't a list of None's fail the check down below?
There was a problem hiding this comment.
yes, it would fail so I added it down there too, because we do want to allow it
| raise TypeError('color must be a string or a list or a dict if labels are provided') | ||
| colors, colors_dict = checkColors(colors, num, labels) | ||
|
|
||
| if labels is not None and len(colors_dict) == 0: |
There was a problem hiding this comment.
I think this section can go away and checkColors doesn't need to return color_dict.
There was a problem hiding this comment.
There is a case where we need colors_dict on line 317 where we make a line graph. We'll have to find a way to adjust that.
|
|
||
| for color in colors: | ||
| if not is_color_like(color): | ||
| if not allowNumbers: |
There was a problem hiding this comment.
I think if allowNumbers, you may need to convert the number to the color in cycle like this:
cycle_colors = plt.rcParams['axes.prop_cycle'].by_key()['color']
color = cycle_colors[color % len(cycle_colors)]
There was a problem hiding this comment.
So the entire loop may look like this:
if isinstance(colors, list):
if len(colors) != num:
raise ValueError('colors should have the length of the set to be colored or satisfy matplotlib color rules')
for i, color in enumerate(colors):
if not is_color_like(color):
if allowNumbers:
cycle_colors = plt.rcParams['axes.prop_cycle'].by_key()['color']
colors[i] = cycle_colors[color % len(cycle_colors)]
else:
raise ValueError('each element of colors should satisfy matplotlib color rules')
....
There was a problem hiding this comment.
I think there's a conversion of the numbers with the color cycle somewhere else, but yes, we could maybe move it here.
There was a problem hiding this comment.
Actually, we use matplotlib.colors.Normalize instead to link up with the cmap. This is on lines 356 and 428
This could somehow be incorporated too though
|
I've no idea whether this is even still a problem that needs fixing, but it should be checked anyway |
3 participants
In the previous version, a 1D array or tuple with the same length as the number of data points would become a matrix with that length in both directions, and indict would get messed up. This was to account for arrays or tuples with RGB or RGBA values.