Allow and test 1's in grid_shape

[cowlicks]

This closes issue #258. And does some small refactoring.

[kwmsmith]

Restarting Travis.

[markkness]

Is this docstring still appropriate? Now that this is no longer test_both_methods.

[cowlicks]

Correct. Thanks @markkness

[kwmsmith]

@cowlicks please add several tests on the client side that use a context with a prime number of targets and create several multi-dimensional distarrays with different distributions in each axis. Check that the grid_shape is as expected.

[cowlicks]

@kwmsmith got it. I limited it to 3 engines because otherwise we'd have to bump of the minimum number of engines on the ipcluster the tests are run with.

[coveralls]

Changes Unknown when pulling 8d563af2378b311e119538aea49910fa58c9baf9 on cowlicks:make-grid-shape-work into * on enthought:master*.

[cowlicks]

Oh hey @coveralls, where have you been?

[markkness]

I am a little concerned that we would no longer have the alternate method available to test against. The previous test_both_methods seemed like a pretty thorough test that mult_partitions worked as expected. I guess that create_factors wouldn't handle the situation of allowing 1 as a factor?

[cowlicks]

Yes. Also, create_factors was unused in the code.

[markkness]

I think create_factors was really a test for mult_partitions, I think probably a simple-but-slow method that mult_partitions could be tested against. So it would have probably more properly lived in the test code.

[cowlicks]

create_factors isn't so simple, it calls out to two other functions. Which themselves should be tested. It is complicated enough to warrant its own tests. And I'm not sure writing meta-tests for functions only used in tests is worthwhile.

I also deleted one of the functions create_factors called, divisors. And another that just wasn't used anywhere.

[kwmsmith]

On Tue, Apr 8, 2014 at 10:05 AM, Blake Griffith notifications@github.comwrote:

In distarray/tests/test_utils.py:

@@ -16,14 +16,18 @@ class TestMultPartitions(unittest.TestCase):
Test the multiplicative parition code.
"""

• def test_both_methods(self):

•    """
  

•    Do the two methods of computing the multiplicative partitions agree?
  

•    """
  

•    for s in [2, 3]:
  

create_factors isn't so simple, it calls out to two other functions.
Which themselves should be tested. It is complicated enough to warrant its
own tests. And I'm not sure writing meta-tests for functions only used in
tests is worthwhile.

Given unlimited resources, we'd do the full testing with multiple versions,
etc. @markkness feel free to make a wishlist issue with your testing
ideas, but otherwise, I'm +1 on just testing the factorisation function in
use and removing the others.

I also deleted one of the functions create_factors called, divisors. And
another that just wasn't used anywhere.

Reply to this email directly or view it on GitHubhttps://github.com//pull/274/files#r11393841
.

Kurt W. Smith, Ph.D. ksmith@enthought.com
Enthought, Inc. http://www.enthought.com | 512.536.1057

[markkness]

I have created issue #293 to note the possible desirability of the extra test of the factoring, and included the relevant code. So I am now satisfied that this PR does not need to keep this anymore.

[markkness]

This PR changes some of the distributions chosen for the example plots. I am not immediately sure if this change is bad or just another acceptable decision. In particular, the examples that were changed:

[With 4 targets]
5x9 Block,Block : Used to be distributed over 2x2 process grid, with this PR now over 1x4 grid.
5x9 Block,Cyclic: Same changes.
5x9 Cyclic,Cyclic: Same changes.
[With 8 targets]
5x9x3 Cyclic,Block,Cyclic: Used to be distributed over 2x2x2 grid, with this PR now over 2x4x1 grid.

Plots with this PR:

Plots with master:

[kwmsmith]

@markkness @cowlicks you should be able to pass in the grid_shape=(2,2) to set the grid shape explicitly, correct? If you don't pass anything in for grid_shape, then one is chosen for you.

@markkness please keep the grid_shape the same as before; no need to change things as far as I can tell.

[markkness]

The plot examples are not currently specifying a grid_shape, they leave it to be chosen. Thus I am not immediately sure if this change is a problem or just another arbitrary selection. It does seem that all of the examples that create an array via the dist keyword to Context.empty() are coming out with a different process grid than before.

[kwmsmith]

@markkness I could have worded my response more clearly: what I meant is that I think you should pass in an explicit grid_shape argument everywhere to ensure they all stay the same, and bypass the logic that selects a grid shape for you.

@cowlicks we'll have to think about what constraints the new grid_shape selection logic should satisfy, especially if it is changing the grid shape from what it was before this PR.

[markkness]

@kwmsmith : Do you think that the arrays created via from_global_dim_data should need the grid_shape specified? (They did not change with this PR, just the ones specified with dist tuple.)

In any case, I am adjusting the example generator to be able to specify a grid shape.

[markkness]

I think I can't specify grid_shape for the from_global_dim_data cases, I think it is implied by the dictionaries.

[markkness]

I have created PR #294 to specify an explicit grid shape in the examples, which then generate the exact same output with this PR as with master.

[cowlicks]

@kwmsmith yes the grid_shape kwarg works for context.empty, ones, zeros but I've never used from_global_dim_data before.

[cowlicks]

When grid_shape is not specified, there is some optimization which takes place that tries to make the number of elements on each processor equal. Since the grid shape can now contain 1's the grid shapes can be further optimized... Which is why this happened.

[bgrant]

Do you think this is not needed? This is here from Brian's original code.

http://docs.scipy.org/doc/numpy/reference/generated/numpy.ndarray.base.html

[kwmsmith]

On Tue, Apr 8, 2014 at 3:01 PM, Mark Kness notifications@github.com wrote:

@kwmsmith https://github.com/kwmsmith : Do you think that the arrays
created via from_global_dim_data should need the grid_shape specified?

Only the arrays created using the convenience methods like ones,
zeros, and empty take a grid_shape argument.

(They did not change with this PR, just the ones specified with disttuple.)

Yes, that's consistent with things as they stand right now. The
from_global_dim_data method is meant to be the "I know what I'm doing,
and I want full control over how to create an array" creation method. The
convenience methods are meant to be easier to use and will fill in more for
you.

In any case, I am adjusting the example generator to be able to specify a
grid shape.

+1. It may make sense to allow a grid_size key in the global dim dicts
and a consistency check to ensure things check out.

Reply to this email directly or view it on GitHubhttps://github.com//pull/274#issuecomment-39894841
.

Kurt W. Smith, Ph.D. ksmith@enthought.com
Enthought, Inc. http://www.enthought.com | 512.536.1057

[kwmsmith]

On Tue, Apr 8, 2014 at 3:04 PM, Mark Kness notifications@github.com wrote:

I think I can't specify grid_shape for the from_global_dim_data cases, I
think it is implied by the dictionaries.

Yes, that is the case -- grid_shape is extracted from the number of
blocks you specify for block dist_type, for example. I think you do have
to specify grid_size for the cyclic distribution. It's extracted
automatically from unstructured, as well.

Reply to this email directly or view it on GitHubhttps://github.com//pull/274#issuecomment-39895113
.

Kurt W. Smith, Ph.D. ksmith@enthought.com
Enthought, Inc. http://www.enthought.com | 512.536.1057

[cowlicks]

rebased

[cowlicks]

Ugh, I can't restart travis.

[markkness]

I restarted it.

[markkness]

The functionality in this PR seems to be fine, as far as I can see. I tried running the test suite with a number of engines other than 4, and this handled it better than master. With 8 engines, master has 15 errors, while this PR has only 10, and none of those are not seen in master. With 11 engines, this PR does as well as with 8, but master gets two more errors.

(Ideally of course make test would work with different engine counts, but that is outside the scope of this.)