Skip to main content
added 103 characters in body
Source Link
AJNeufeld
  • 34k
  • 5
  • 39
  • 101

generateArray() is a terrible name for the function. Maybe generateMaskFromImage()?


The output arr is not being used by your code. If you omitted the line:

The output arr is not being used by your code. If you omitted the line:

generateArray() is a terrible name for the function. Maybe generateMaskFromImage()?


The output arr is not being used by your code. If you omitted the line:

Added lookup of the [0]'th pixel coordinate
Source Link
AJNeufeld
  • 34k
  • 5
  • 39
  • 101
def generateArray():
    arr = []
    for y in range(height):
        row = []
        for x in range(width):
            if img[y][x]img[y][x][0] != 1:
               row.append(1)
            else:
               row.append(0)
        arr.append(row)
    return arr
def generateArray():
    arr = [None] * height          # Correctly sized array of rows
    for y in range(height):
        row = [0] * width          # Correctly sized row, filled with 0's
        for x in range(width):
            if img[y][x]img[y][x][0] != 1:
               row[x] = 1
        arr[y] = row
    return arr
        for x in range(width):
            if img[y][x]img[y][x][0] != 1:
                row[x] = 1

which loops over all the x values of a row, and for every x value, looks up the row img[y], and then the pixel in the row img[y][x], for each pixel in that row, and then accesses the [0] element of that pixel. That's a lot of lookups. If instead we used:

        for x, pixel in enumerate(img[y]):
            if pixelpixel[0] != 1:
                row[x] = 1
def generateArray():
    arr = [None] * height
    for y in range(height):
        arr[y] = [ 1 if pixelpixel[0] != 1 else 0 for pixel in img[y] ]
    return arr
def generateArray():
    return [ [ 1 if pixelpixel[0] != 1 else 0 for pixel in row ] for row in img ]
def generateArray():
    arr = []
    for y in range(height):
        row = []
        for x in range(width):
            if img[y][x] != 1:
               row.append(1)
            else:
               row.append(0)
        arr.append(row)
    return arr
def generateArray():
    arr = [None] * height          # Correctly sized array of rows
    for y in range(height):
        row = [0] * width          # Correctly sized row, filled with 0's
        for x in range(width):
            if img[y][x] != 1:
               row[x] = 1
        arr[y] = row
    return arr
        for x in range(width):
            if img[y][x] != 1:
                row[x] = 1

which loops over all the x values of a row, and for every x value, looks up the row img[y], and then the pixel in the row img[y][x], for each pixel in that row. That's a lot of lookups. If instead we used:

        for x, pixel in enumerate(img[y]):
            if pixel != 1:
                row[x] = 1
def generateArray():
    arr = [None] * height
    for y in range(height):
        arr[y] = [ 1 if pixel != 1 else 0 for pixel in img[y] ]
    return arr
def generateArray():
    return [ [ 1 if pixel != 1 else 0 for pixel in row ] for row in img ]
def generateArray():
    arr = []
    for y in range(height):
        row = []
        for x in range(width):
            if img[y][x][0] != 1:
               row.append(1)
            else:
               row.append(0)
        arr.append(row)
    return arr
def generateArray():
    arr = [None] * height          # Correctly sized array of rows
    for y in range(height):
        row = [0] * width          # Correctly sized row, filled with 0's
        for x in range(width):
            if img[y][x][0] != 1:
               row[x] = 1
        arr[y] = row
    return arr
        for x in range(width):
            if img[y][x][0] != 1:
                row[x] = 1

which loops over all the x values of a row, and for every x value, looks up the row img[y], and then the pixel in the row img[y][x], for each pixel in that row, and then accesses the [0] element of that pixel. That's a lot of lookups. If instead we used:

        for x, pixel in enumerate(img[y]):
            if pixel[0] != 1:
                row[x] = 1
def generateArray():
    arr = [None] * height
    for y in range(height):
        arr[y] = [ 1 if pixel[0] != 1 else 0 for pixel in img[y] ]
    return arr
def generateArray():
    return [ [ 1 if pixel[0] != 1 else 0 for pixel in row ] for row in img ]
Source Link
AJNeufeld
  • 34k
  • 5
  • 39
  • 101

Yes, there are a few things which can be done to speed up your code.

You create local variables width and height. But then you don't use these variables. Instead you use for x in range(img.shape[1]): which ends up evaluating img.shape[1] a total of height times. Consider that img.shape retrieves the shape array from img, and then accesses the [1] member of that list. Using for x in range(width): doesn't have that overhead.

Consider arr[y].append(_). Again, this code is looking up in the arr list, the [y] element, and to that retrieved list appending a value. That done once for every pixel. If you kept a handle to the list that you are appending to, you wouldn't have to look it up each time.

def generateArray():
    arr = []
    for y in range(height):
        row = []
        for x in range(width):
            if img[y][x] != 1:
               row.append(1)
            else:
               row.append(0)
        arr.append(row)
    return arr

The above code should be a little faster.

Appending is a time-consuming operation. If the list is stored as an array, rather than a linked-list, then appending an element will require frequent reallocation of the array capacity, and copying of all of the elements to the new storage area. We can prevent this reallocating and copying by allocating storage for all of the data all at once:

def generateArray():
    arr = [None] * height          # Correctly sized array of rows
    for y in range(height):
        row = [0] * width          # Correctly sized row, filled with 0's
        for x in range(width):
            if img[y][x] != 1:
               row[x] = 1
        arr[y] = row
    return arr

Here, the row array starts off with [0, 0, 0, 0, ..., 0, 0, 0] so it is only necessary to set the elements to 1 where needed.


Looping and list lookup can be slow. Consider:

        for x in range(width):
            if img[y][x] != 1:
                row[x] = 1

which loops over all the x values of a row, and for every x value, looks up the row img[y], and then the pixel in the row img[y][x], for each pixel in that row. That's a lot of lookups. If instead we used:

        for x, pixel in enumerate(img[y]):
            if pixel != 1:
                row[x] = 1

we'd be iterating over the row of pixels, getting their values without the double list lookup.

We could also use list comprehension to build the list. Since each row is of a fixed, known length, the list comprehension can allocate the correctly sized list, and fill in the elements one-at-a-time, without indexing or appending.

def generateArray():
    arr = [None] * height
    for y in range(height):
        arr[y] = [ 1 if pixel != 1 else 0 for pixel in img[y] ]
    return arr

That also applies to building each row.

def generateArray():
    return [ [ 1 if pixel != 1 else 0 for pixel in row ] for row in img ]

The function generateArray() uses a global variable img. It would be better to pass img into the function.

def generateArray(img):
    # code which uses img

The output arr is not being used by your code. If you omitted the line:

arr = generateArray()

your code would run much, much faster. ;-)