bug fix on color handling for showProjection #1070
Conversation
jamesmkrieger
force-pushed
the
devel-5
branch
3 times, most recently
from
3da94bb to
133a7cc
Compare
jamesmkrieger
force-pushed
the
devel-5
branch
from
eda4865 to
f1f296f
Compare
prody/dynamics/plotting.py
Outdated
| colors = [colors_dict[label] for label in labels] | ||
|
|
||
| if isinstance(colors, list): | ||
| if len(colors) != num and not is_color_like(colors): |
There was a problem hiding this comment.
I don't think we need and not is_color_like(colors) here since if it was color-like, line 2371 would have converted it to a list of colors (of num) already (correct me if I am wrong).
There was a problem hiding this comment.
ok, yes, I agree
prody/dynamics/plotting.py
Outdated
| if np.any([not is_color_like(color) for color in colors]): | ||
| if not allowNumbers: | ||
| raise ValueError('each element of colors should satisfy matplotlib color rules') | ||
| elif np.any([not isinstance(color, Number) for color in colors]): |
There was a problem hiding this comment.
same as above too
| 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
prody/dynamics/plotting.py
Outdated
| if not isinstance(color, type(colors[0])): | ||
| raise TypeError('each element of colors should have the same type') | ||
| else: | ||
| raise TypeError('colors should be a colour spec or convertible to a list of color specs') |
There was a problem hiding this comment.
Nitpick: colour spec -> color spec for style consistency.
| colors = [colors_dict[label] for label in labels] | ||
| else: | ||
| 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
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.