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()
asked Jul 16, 2019 at 15:56
\$\endgroup\$

2 Answers 2

2
\$\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 ...
answered Jul 18, 2019 at 7:51
\$\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.

answered Jul 16, 2019 at 18:51
\$\endgroup\$

Your Answer

Draft saved
Draft discarded

Sign up or log in

Sign up using Google
Sign up using Email and Password

Post as a guest

Required, but never shown

Post as a guest

Required, but never shown

By clicking "Post Your Answer", you agree to our terms of service and acknowledge you have read our privacy policy.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.