How can this verbose, unpythonic routine be improved?

Question

Is there a more pythonic way of doing this? I am trying to find the eight neighbours of an integer coordinate lying within an extent. I am interested in reducing its verbosity without sacrificing execution speed.

def fringe8((px, py), (x1, y1, x2, y2)):
    f = [(px - 1, py - 1),
         (px - 1, py),
         (px - 1, py + 1),
         (px, py - 1),
         (px, py + 1),
         (px + 1, py - 1),
         (px + 1, py),
         (px + 1, py + 1)]
    f_inrange = []
    for fx, fy in f:
        if fx < x1:  continue
        if fx >= x2: continue
        if fy < y1:  continue
        if fy >= y2: continue
        f_inrange.append((fx, fy))
    return f_inrange

Answer 1

This list comprehension should work

f_inrange = [(px+i,py+j) for i in (-1,0,1) for j in (-1,0,1) \
             if (i != j or i != 0) and x1 <= px+i < x2 and \
             y2 > py+j >= y1]

though it is kind of lengthy.

Answer 2

Here's a thought:

def fringe8((px, py), (x1, y1, x2, y2)):
    f = [(px + dx, py + dy) for (dx, dy) in itertools.product((-1,0,1),repeat=2) if not (dx == dy == 0)]
    f_inrange = [(fx, fy) for (fx, fy) in f if x1 <= fx < x2 and y1 <= fy < y2]
    return f_inrange

Answer 3

Here's my take on the code cleanup:

Edit: I've taken David's code into my answer to make it even more compact (and faster execution time).

>>> from itertools import product
>>>
>>> def fringe8((px, py), (x1, y1, x2, y2)):
...     f = [(px+dx, py+dy) for (dx, dy) in product((-1,0,1),(-1,0,1)) if (dx, dy) != (0, 0)]
...     f_inrange = [(fx, fy) for (fx, fy) in f if x1 <= fx < x2 and y1 <= fy < y2]
...     return f_inrange
...   
>>> fringe8((2, 2), (1, 1, 3, 3))
[(1, 1), (1, 2), (2, 1)]

Edit: If you're not too comfortable with list comprehensions, feel free to break it down to a for loop. The conditions proposed here, and others' answers, are all much more concise for you to use.

After all, one the goals of list comprehesion is to make it easier to read, and not verbose.

Edit again: Please also look at Ryan Ginstrom's answer.

Answer 4

def fringe8((px, py), (x1, y1, x2, y2)):
    return [(x, y) for x,y 
            in itertools.product(   [xi+px for xi in [-1,0,1]],
                                    [yi+py for yi in [-1,0,1]] )
            if (x1<=x<x2) and (y1<=y<y2) and not ((x==px) and (y==py))]

I don't know if that's more pythonic, but it's certainly another way to do it.

Answer 5

def fringe8((px, py), (x1, y1, x2, y2)):
  return [(fx, fy) for fx in [px - x for x in [-1, 0, 1]] if (fx >= x1 and fx < x2) 
  for fy in [py - y for y in [-1, 0, 1]]  if fy >= y1 and fy < y2 
  and not(fx == px and fy == py)]

Answer 6

here is my recycle :

def fringe8((px, py), (x1, y1, x2, y2)):
    return [(x, y) for x,y in itertools.product((px-1, px , px+1),
                                                (py-1, py , py+1))
            if (x1<=x<x2) and (y1<=y<y2) and (x,y) != (px,py)]

Answer 7

This is a reworking of Xavier Ho's answer. I think that it's made a little more clear by using intermediate steps.

from itertools import product

def fringe8((px, py), (x1, y1, x2, y2)):

    nonzero = (pair for pair in product((-1,0,1),(-1,0,1)) if pair != (0, 0))
    f = ((px+dx, py+dy) for (dx,dy) in nonzero)

    def in_range((fx, fy)):
        return x1 <= fx < x2 and y1 <= fy < y2
    return [pair for pair in f if in_range(pair)]

print fringe8((2, 2), (1, 1, 3, 3))

Answer 8

The name of the function could be more informative. I know what the code does, but I have no idea what it is intended for.

Answer 9

Here's my effort. It constructs the interesting ranges directly, so doesn't test redundant points (except for the pesky center point). I added an extra argument r than can return a larger square around the point too.

def fringe((px, py), (x1, y1, x2, y2), r=1):
    return [(x, y)
            for x in xrange(max(px - r, x1), min(px + r + 1, x2))
            for y in xrange(max(py - r, y1), min(py + r + 1, y2))
            if x != px or y != py]

Answer 10

I don't know from pythonicness, but I see that this function is doing two things: a) finding adjacent points; and b) restricting a list of points to those that are in range. So I would split it up, because a) it's clearer; b) sometimes I might want one of those behaviors without the other; c) it's easier to test; and d) it's easier to name. I won't attempt to convert this advice into python - I'm sure my attempt wouldn't be pretty - but that would be my starting point.

Answer 11

I like Carl's suggestion. Also you can avoid repeated construction of the neighbor offsets.

from itertools import product

NEIGHBOR_OFFSETS = [pair for pair in product((-1,0,1), (-1,0,1))
    if pair != (0, 0)]

def clip(points, (x1, y1, x2, y2)):
    return [(px, py) for px, py in points if 
        x1 <= px < x2 and y1 <= py < y2]

def neighbors(px, py):
    return [(px+dx, py+dy) for dx, dy in NEIGHBOR_OFFSETS]

def fringe8(point, rect):
    return clip(neighbors(*point), rect)