Asymmetry in periodic structure finding.

[tomr-stargazer]

I noticed some funny behavior with computing periodic structures (partially noted in #123, which is a bit of a mess), and honed it down to this. test_periodic_left is failing for me while test_periodic_right passes.

def test_periodic_left():
    x = np.array([[1, 0, 0, 0, 0],
                  [1, 0, 0, 0, 1],
                  [1, 0, 0, 0, 0]])
    d = Dendrogram.compute(x, min_value=0.5,
                           neighbours=periodic_neighbours(1))
    expected = np.array([[0, -1, -1, -1, -1],
                         [0, -1, -1, -1, 0],
                         [0, -1, -1, -1, -1]])
    np.testing.assert_array_equal(d.index_map, expected)

E                   AssertionError:
E                   Arrays are not equal
E
E                   (mismatch 13.3333333333%)
E                    x: array([[ 0, -1, -1, -1, -1],
E                          [ 1, -1, -1, -1,  0],
E                          [ 2, -1, -1, -1, -1]], dtype=int32)
E                    y: array([[ 0, -1, -1, -1, -1],
E                          [ 0, -1, -1, -1,  0],
E                          [ 0, -1, -1, -1, -1]])

@ChrisBeaumont, do you have any insight as to why this might be the case? Perhaps some uncaught edge-case in periodic_neighbours? (also, sorry for the flurry of recent pull requests - I appreciate whatever time you've been able to devote to helping me out.)

[tomr-stargazer]

I added two more simple symmetric tests, and now it's test_periodic_right_narrow that fails:

def test_periodic_right_narrow():
    x = np.array([[0, 0, 0, 0, 0],
                  [1, 0, 0, 1, 1],
                  [0, 0, 0, 0, 0]])
    d = Dendrogram.compute(x, min_value=0.5,
                           neighbours=periodic_neighbours(1))
    expected = np.array([[-1, -1, -1, -1, -1],
                         [0, -1, -1, 0, 0],
                         [-1, -1, -1, -1, -1]])
    np.testing.assert_array_equal(d.index_map, expected)

E                   AssertionError:
E                   Arrays are not equal
E
E                   (mismatch 13.3333333333%)
E                    x: array([[-1, -1, -1, -1, -1],
E                          [ 0, -1, -1,  1,  1],
E                          [-1, -1, -1, -1, -1]], dtype=int32)
E                    y: array([[-1, -1, -1, -1, -1],
E                          [ 0, -1, -1,  0,  0],
E                          [-1, -1, -1, -1, -1]])

[ChrisBeaumont]

I think periodic_neighbors has a bug:

@@ -701,9 +701,9 @@ def periodic_neighbours(axes):

     def _wrap(c, shp):
         # note: shp is padded along each dimension,
-        #      so values to wrap occur 0, len-1
+        #      so values to wrap occur -1, len-1
         for a in axes:
-            if c[a] == 0:
+            if c[a] < 0:
                 c[a] = shp[a] - 2
             elif c[a] == shp[a] - 1:
                 c[a] = 0

That change should fix it

[tomr-stargazer]

Awesome -- thanks @ChrisBeaumont. The tests now pass on my machine. If there are no objections to keeping in these four new tests (I tried to make them short and unit-like) then this should be merged ASAP.

[ChrisBeaumont]

Looks good, thanks!