-
-
Notifications
You must be signed in to change notification settings - Fork 2.7k
Allow more than 2 colormaps again #493
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
hey - you might be right, I'll take a look through the changes now and add
thoughts
On Mon, May 30, 2016 at 11:30 AM, Adam Kulidjian notifications@github.com
wrote:
@choldgraf https://github.com/choldgraf @theengineear
https://github.com/theengineearGuys,
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)
You can view, comment on, or merge this pull request online at:
#493
Commit Summary
- Allow more than 2 colormaps again
File Changes
- M plotly/tools.py
https://github.com/plotly/plotly.py/pull/493/files#diff-0 (88)Patch Links:
- https://github.com/plotly/plotly.py/pull/493.patch
- https://github.com/plotly/plotly.py/pull/493.diff
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#493, or mute the thread
https://github.com/notifications/unsubscribe/ABwSHZw0SxBrl3GPFWOhKllpCvlt8Xjeks5qGyzbgaJpZM4Ip-KO
.
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
plotly/tools.py
Outdated
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.
🎎 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.
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 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
On Jun 2, 2016 2:31 PM, "Andrew" notifications@github.com wrote:
@Kully https://github.com/Kully I'm happy with it now :) 💃 from me.
@choldgraf https://github.com/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).—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#493 (comment), or mute
the thread
https://github.com/notifications/unsubscribe/ABwSHa5DVYzWqqXjgkAop1Gjw_sl1t3Wks5qHz28gaJpZM4Ip-KO
.
@Kully I'm happy with it now :) 💃 from me.
I don't think we can do that. Or maybe I'm not reading this right but...
int(t / (1./(len(colormap) - 1))) isn't the same as int(1 / (1./(len(colormap) - 1)))
I think the t does make the difference.
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.
🎉 🔔 🎆
💃 💃 👯 !
@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)