Allow more than 2 colormaps again #493
Conversation
|
hey - you might be right, I'll take a look through the changes now and add On Mon, May 30, 2016 at 11:30 AM, Adam Kulidjian notifications@github.com
|
| @@ -1468,9 +1468,9 @@ def _find_intermediate_color(lowcolor, highcolor, intermed): | |||
| diff_1 = float(highcolor[1] - lowcolor[1]) | |||
| diff_2 = float(highcolor[2] - lowcolor[2]) | |||
|
|
|||
| inter_colors = np.array([lowcolor[0] + intermed * diff_0, | |||
There was a problem hiding this comment.
What is the benefit gained by making it a list and looping through? IME using arrays instead of lists almost always makes things much faster.
There was a problem hiding this comment.
What is the benefit gained by making it a list and looping through? IME using arrays instead of lists almost always makes things much faster.
I don't necessarily think it's faster or anything like that. I actually just switched it back to this because I wanted it to work with my FF converting code
There was a problem hiding this comment.
I'm wondering if there's a benefit other than speed. E.g., if you thought the user might pass in tuples of variable shapes then it's probably better for the input to be a list or tuple or something (since arrays require consistency across dimensions). If you like, maybe I can take a pass through and make it work w/ arrays similar to the last PR. I don't think the syntax would be any different, and it'll likely be an order of magnitude faster to use arrays.
| for i, j, k in t_colors.T] | ||
| return labelled_colors | ||
| # find the normalized distance t of a triangle face between vmin and | ||
| #vmax where the distance is normalized between 0 and 1 |
There was a problem hiding this comment.
🎎 missing a in this comment.
|
Sorry to drag this one out longer on you with this review @Kully, ping me directly when you want another review and I can hop on it quickly. |
|
|
||
| return color | ||
| return face_color |
There was a problem hiding this comment.
💥 so much more readable! thanks!
|
@Kully I'm happy with it now :) 💃 from me. @choldgraf unless there is something jarringly wrong about this still, can we merge it? That way, you could open up a new PR to improve computation speed again (with these new test constraints). |
|
I say go for it! Sent from my phone, apologies for the curtness and type-os
|
I don't think we can do that. Or maybe I'm not reading this right but... I think the |
| face_color = colormap[low_color_index] | ||
| if face == vmax: | ||
| # pick last color in colormap | ||
| face_color = colormap[-1] |
|
💃 💃 👯 ! |
@choldgraf @theengineear
Guys,
So Chris, I think when you made the efficiency improvements to trisurf, you got rid of the capability of mapping divergent or colormaps of more than 2 colors. I have put that feature back now while trying to maintain the efficiency as best I could.
I put the 'rgb(%s, %s, %s)' stuff in the FigureFactory function _label_rgb(), which I was using anyways to convert a tuple to an rgb tuple.
Also, facecolor is not done in one line anymore, but is an empty list that keeps appending.
Let me know if you have any improvements to add.
(Also rewrote some doc strings for clarity)