Speed up 3 slow tests.#369
Conversation
There was a problem hiding this comment.
This is kindof cheating, isn't it? :) I'd prefer us to keep the bounds the same and find a better and faster way to do the assignment rather than the for loops below.
There was a problem hiding this comment.
the for loops can be replaced with fromarray. But I don't see any reason to make this test so large.
There was a problem hiding this comment.
On Mon, May 12, 2014 at 11:17 AM, Blake Griffith
notifications@github.comwrote:
In distarray/dist/tests/test_distarray.py:
def test_from_global_dim_data_uu(self):
rows = 6cols = 20rows = 3cols = 10the for loops can be replaced with fromarray.
+1.
But I don't see any reason to make this test so large.
Under what definition of "large" is a 120-element array considered so? We
should have a few 106 element arrays in the normal test suite for some
small scaling tests. The performance and profiling test suite should work
with 109 element arrays routinely to make obvious any communication or
performance bottlenecks.
Please keep this test at 120 elements at least, and when you or someone
else implements it on top of fromarray with slicing, we should bump that
number up a few orders of magnitude.
—
Reply to this email directly or view it on GitHubhttps://github.com//pull/369/files#r12538308
.
Kurt W. Smith, Ph.D. ksmith@enthought.com
Enthought, Inc. http://www.enthought.com | 512.536.1057
There was a problem hiding this comment.
All these from_global_dim_data_<distribution> tests are indexing with for loops like this. Should this be changed for the all?
There was a problem hiding this comment.
They are also filling up the whole array with unique values, then not doing anything with them...? I suppose these tests are also trying to test that ever element can be indexed too.
But if we just want to test that we can make a distarray with a given distribution and index all its elements we can do:
- create distribution
- create distarray
- call distarray.tondarraay
Should I change the tests to work like this?
|
@cowlicks what's the overall effect on peformance with these changes? |
|
@kwmsmith here is before remote I'm currently trying to generate the after images. Also, sorry these are images, the nose plugin uses color escape codes which I have not been able to remove, so piping the output to a text file is unreadable. |
|
I've added changes to more tests to this PR. See 23d7157 |
|
Changes mostly affect And on master:
|
There was a problem hiding this comment.
I presume the toarray() call is equivalent to the for loop above?
There was a problem hiding this comment.
Yes but not exactly. It iterates over the whole array (like the for loop) and calls __getitem__. But it all happens on the client.
The for loop has a round trip for every element and calls __setitem__.
30 element array instead of 120.
speed things up by replacing for loops over arrays with calls to
.toarray(). Which still tests that the array can be indexed, but it
pulls it to the client and does it all on the client instead of
remotely.
This change affects:
* test_from_global_dim_data_irregular_block
* test_from_global_dim_data_bu
* test_from_global_dim_data_uu
* test_from_global_dim_data_bc
* test_irregular_block_assignment
|
LGTM. I did similar things in #375, so there may be a minor conflict. |
3 participants
No description provided.