vectorizing some trisurf functions for performance improvement

[choldgraf]

First pass at optimizing some of the trisurf machinery. This basically just vectorizes things where it used to use list comprehensions / loops. I also tried to make functions accept a vector of inputs rather than a single number as an input to reduce redundant computation etc. Might have missed some things, but for me this brought the _trisurf function from ~16 seconds to ~2 seconds (!).

We should make sure that the outputs are the same, but I ran nosetests on test_figure_factory.py and it passed for me. If you all like the changes, then I can clean up the comments and change docstrings as necessary.

WDYT? @jackparmer @Kully (couldn't find Emilia's handle w/ a quick search, sorry if I'm missing other folks)

[empet]

@choldgraf My suggestion to reduce execution time referred to an initial code for trisurf. Here that proposal amounts to changing the lines 1575-1593 as follows:

if x_edge is None:
    x_edge =[]
if y_edge is None:
    y_edge = []   
if z_edge is None:
    z_edge = []  
for T in tri_vertices:
        x_edge+=[T[k%3][0] for k in range(4)]+[ None]
        y_edge+=[T[k%3][1] for k in range(4)]+[ None]
        z_edge+=[T[k%3][2] for k in range(4)]+[ None]    

Emilia

[choldgraf]

Good point - I've made these edits w/ a few modifications to vectorize. I think it should be doing the same thing, but someone should double check to make sure that I didn't change something. I compared the new vs old codebase and it seems like the outputs are pretty much the same (with one difference being the call to lines and Mesh3d now contains arrays of data rather than lists of data (but that'd be an easy fix if that's not good).

Without lines plotting the speedup went from ~17 seconds to ~2-3 seconds
With lines plotting the speedup went from ~40 seconds to ~2-4 seconds

However, I also noticed that there aren't any tests that cover trisurf, so I suppose it doesn't matter that tests still pass :) that should probably be done, but I don't have time left today to write a full set of tests for the function...

[jackparmer]

Nice! Thanks @choldgraf and @empet ! 💃 from me

[Kully]

However, I also noticed that there aren't any tests that cover trisurf, so I suppose it doesn't matter that tests still pass

I can start writing tests.

[choldgraf]

Sounds good, lmk if I can help. Do you want tests in this PR or in another?
Also I think it's worth raising a warning at least when someone tries to
plot this many triangles with the edges. Thus far it always crashed my
browser.

Sent from my phone, apologies for the curtness and type-os
On May 20, 2016 12:52 PM, "Adam Kulidjian" notifications@github.com wrote:

However, I also noticed that there aren't any tests that cover trisurf, so
I suppose it doesn't matter that tests still pass

I can start writing tests.

—
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#472 (comment)

[yankev]

cool logic.

[yankev]

This looks good to me, the logic holds up, and with some speed increases for the few examples I tried out. Going to wait for those tests for a more exhaustive look.

[Kully]

This looks good to me, the logic holds up, and with some speed increases for the few examples I tried out. Going to wait for those tests for a more exhaustive look.

I'm getting some test failures. Checking it out now.

[choldgraf]

cool - if you push the tests to master then I can rebase and change things
until they pass

On Tue, May 24, 2016 at 2:13 PM, Adam Kulidjian notifications@github.com
wrote:

This looks good to me, the logic holds up, and with some speed increases
for the few examples I tried out. Going to wait for those tests for a more
exhaustive look.

I'm getting some test failures. Checking it out now.

—
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#472 (comment)

[Kully]

cool - if you push the tests to master then I can rebase and change things
until they pass

So the test failure was just because I hadn't updated my expected tri_surf plot for the test_trisurf_kwargs().

I am currently working on another PR #477 where I have allowed for a list of more than 2 colors to pass into trisurf. What exactly should I be writing tests for here and can I do it here without worrying about merging this Plus_2_Colorscale PR I have?

[theengineear]

However, I also noticed that there aren't any tests that cover trisurf

@choldgraf and @Kully There is a test that covers trisurf output:

https://github.com/plotly/plotly.py/blob/master/plotly/tests/test_optional/test_figure_factory.py#L668-L754

I'd imagine that this test would be sufficient for now. @Kully if all the current tests are passing, can we merge it?

[Kully]

New PR with passing tests:

#481

[choldgraf]

Sounds good all - after this one I'll take comments to the new PR.

@theengineear I don't know how I missed those trisurf tests, I just did a ctrl-f for trisurf before, but missed it somehow. Good to see tests in there!