Fix and simplify filter_adjlist. #244
Conversation
The docs say it should remove elements in insertion order, but it doesn't because `set` is unordered. It is dependent on the order Python decides, which does in fact change between versions, and the related test breaks on 3.8. The test itself is also wrong because removed elements in a 'nice' manner, not insertion order.
| to_drop_mask = ((adjlist[focal_col] == tupe[0]) | ||
| & (adjlist[neighbor_col] == tupe[-1])) | ||
| to_drop = adjlist[to_drop_mask] | ||
| elif (tuple(reversed(tupe)) in undirected): |
There was a problem hiding this comment.
This condition was redundant with https://github.com/pysal/libpysal/pull/244/files#diff-ab25b3d21ac8613bd5cb0998c1f9f2a7L197 which added the reverse to the set.
| else: | ||
| undirected.update((tupe,)) | ||
| undirected.update((tuple(reversed(tupe),))) |
There was a problem hiding this comment.
But there was a bug here as the comma was within the tuple call, so it really added the two elements to the set, not a reversed tuple.
| @@ -34,8 +34,8 @@ def test_filter(self): | |||
| badgrid = weights.W.from_adjlist(alist) | |||
| np.testing.assert_allclose(badgrid.sparse.toarray(), | |||
| grid.sparse.toarray()) | |||
| assert set(alist.focal.unique().tolist()) == set(list(range(4))) | |||
There was a problem hiding this comment.
The adjacency list as created in the top of this test is:
focal neighbor weight
0 0 2 1.0 *
1 0 1 1.0 *
2 1 0 1.0
3 1 3 1.0 *
4 2 0 1.0
5 2 3 1.0 *
6 3 1 1.0
7 3 2 1.0
I have marked the unique edges in order with*. As you can see, the documented behaviour could not have produced {0, 1, 2, 3} for either focal or neighbor.
Possibly this part of the test should be dropped entirely, because the next bits test what is actually true, that the edges are unique (not necessarily the vertices).
There was a problem hiding this comment.
Sure. This test is ok to drop. The main point was to test the edges.
| @@ -34,8 +34,8 @@ def test_filter(self): | |||
| badgrid = weights.W.from_adjlist(alist) | |||
| np.testing.assert_allclose(badgrid.sparse.toarray(), | |||
| grid.sparse.toarray()) | |||
| assert set(alist.focal.unique().tolist()) == set(list(range(4))) | |||
There was a problem hiding this comment.
Sure. This test is ok to drop. The main point was to test the edges.
nosetestson your changes?pysal/masterbranch.filter_adjustshould remove elements in insertion order, but it doesn't do that becausesetis unordered. It is dependent on the order Python decides, which does in fact change between versions, and the related test breaks on 3.8.The test itself is also wrong because it removed elements in a 'nice' manner, not insertion order.