2
\$\begingroup\$

As a practice exercise, I implemented a cellular automaton (Conway's Game of Life) in python (with help from this tutorial).

I am very curious, if this code follows common best practices and I am wondering how to improve the code in terms of performance and readability.

import numpy as np
import matplotlib.pyplot as plt
from matplotlib import animation

arr = []

size_grid_x = 6
size_grid_y = 6
universe = np.zeros((size_grid_x, size_grid_y))
new_universe = np.copy(universe)

beacon = [[1, 1, 0, 0],
          [1, 1, 0, 0],
          [0, 0, 1, 1],
          [0, 0, 1, 1]]

universe[1:5, 1:5] = beacon


def update(universe, x, y):
    X = size_grid_x
    Y = size_grid_y

    neighbors = lambda x, y: [(x2, y2) for x2 in range(x - 1, x + 2)
                              for y2 in range(y - 1, y + 2)
                              if (-1 < x <= X and
                                  -1 < y <= Y and
                                  (x != x2 or y != y2) and
                                  (0 <= x2 <= X) and
                                  (0 <= y2 <= Y))]

    num_neighbours = sum([universe[i] for i in neighbors(x, y)])
    new_val = universe[x, y]

    if universe[x, y] and not 2 <= num_neighbours <= 3:
        new_val = 0

    elif num_neighbours == 3:
        new_val = 1
    return new_val


for i in range(100):
    for x in range(5):
        for y in range(5):
            new_universe[x, y] = update(universe, x, y)

    universe[:, :] = new_universe[:, :]
    arr.append(np.copy(universe))

fig = plt.figure()
i = 0
im = plt.imshow(arr[0], animated=True)


def update_figure(*args):
    global i
    if i < 99:
        i += 1
    else:
        i = 0
    im.set_array(arr[i])
    return im,


ani = animation.FuncAnimation(fig, update_figure, blit=True)
plt.show()
\$\endgroup\$
1
\$\begingroup\$

lambda

If you want to give the function a name, then it shouldn't be a lambda. They are generally good for very short functions to pass as an argument to another function. For example, sorted(list_of_tuples, key=lambda t:t[1]) to sort on based on the second element of a tuple.

comprehensions

Long multi-line comprehensions can be hard to read. In 6 months will you remember why the range is (x-1, x+2) and what all the inequalities are checking. For things like this, I find something like this more readable:

def count_neighbors(x, y):
    '''
    Counts the number of living cells in the neighborhood of (x,y)
    '''
    xy = [(x-1, y+1), ( x, y+1), (x+1, y+1),
          (x-1, y  ),            (x+1, y+0),
          (x-1, y-1), ( x, y-1), (x-1, y+1)]

    return sum(universe(nx, ny) for nx,ny in xy if 0<=nx<size_grid_x and 0<=ny<size_grid_y)

sum() takes an iterable

So you don't need to make a list first. Just pass in the generator, a la:

num_neighbours = sum(universe[i] for i in neighbors(x, y))

logic

I took awhile to understand the logic for setting new_val. Add some comments to explain the logic. Maybe rearrange it to make it easier to understand when you look at it in a year:

if num_neighbors == 2:
    new_val = universe[x,y]     # no change

elif num_neighbors == 3:
    new_val = 1                 # birth (or no change if already a 1)

else:
    new_val = 0                 # death  

numpy and scipy

These libraries are full of functions for operating on arrays. You could write this using np.where and scipy.ndimage.convolve like so:

import numpy as np
from scipy.ndimage import convolve

... same setup for universe, beacon, etc...

kernel = np.array([[1,1,1],
                   [1,0,1],
                   [1,1,1]])

for i in range(100):
    neighbors = convolve(universe, kernel, mode='constant')

    alive = np.logical_or(np.logical_and(universe==1, neighbors==2), neighbors==3)

    universe = np.where(alive, 1, 0)

    arr.append(np.copy(universe))

... same plotting code ...
\$\endgroup\$
2
\$\begingroup\$

I'm on my phone, so I can't write anything too elaborate. I'll just make some notes:


I don't think you're making good use of functions here. For the most part, you have everything tucked inside of update. I think that function is far too large, and doing too much explicitly. I would, for example, create a function called should_live or something similar that is passed the cell's current state and number of neighbors, and returns if the cell should be alive the next tick or not.

You do break some of that functionality off into a local function neighbors, but I don't think it's ideal the way you've done it. I would make that an external function, and I would call it n_neighbors or count_neighbors or something similar to make it clearer that it's a function that counts neighbors, not a collection of neighbors. You could also clean up that long list comprehension guard by creating a is_inbounds function that checks if a point is i bounds or not.

I'd also rename update, as is isn't actually updating anything. It's gathering information then returning a decision.


I would not have code top-level like you have it here. Having the code run just because you loaded it will get annoying if you ever change your workflow to involve a REPL. I would tuck the routine into a main function, then maybe call that function from a if __name__ == "__main__" block.


In short, I'd break your code up more and be more cautious when it comes to naming.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.