Skip to main content
Code Review

Return to Answer

Fixed indexing error
Source Link
DavidW
DavidW
  • 141
  • 5

There's quite a bit of code here so I'm not going to be comprehensive, but here are some observations.

Profiling

The first thing you should do is to try to profile your code and work out where time is actually being spent. Concentrate your optimization in these functions.

get26Neighbors

I doubt if this function is limiting your speed. However, np.delete is often a little slow since it involves moving elements about in an already allocated array. I'd be tempted to allocate an empty array and fill it instead:

cdef np.ndarray[np.int_t, ndim=2] neighbors = np.empty((26,3),dtype=np.int)
neighbors[:12,0] = xx[:12]
neighbors[14neighbors[12:,0] = xx[14xx[13:]
neighbors[:12,1] = yy[:12]
# etc...
# (check my maths on which element to skip)

getNeighborLabels

Indexing using arr[tuple(neighbor_index)] will be slow. In order to take advantage of Cython's ability to index arrays quickly you need to index using only integers and slices, not general Python objects like tuple. It's better to explicitly spell out the indices (event if this is a little more code):

for i in range(neighbors.shape[0]):
    xi = neighbors[i,0]
    yi = neighbors[i,1]
    zi = neighbors[i,2]
    if arr[xi,yi,zi] == 1 and labels[xi,yi,zi] != 0:
        neighborLabels.append(labels[xi,yi,zi])

I've also changed the outer loop to an integer indexed loop since this is much quicker in Cython than using the Python iterator protocol (for x in neighbours) - I know this is generally considered worse style in Python, but it is faster here. There's other places you could apply this.

I'd also be tempted to make neighborLabels a list rather than a numpy array, since Python lists are designed to have quick appends while numpy arrays aren't.

General comments on typing

You've specified explicit types for (almost) everything. You don't generally get much benefit in Cython from specifying basic Python types such as dict or tuple. You also don't benefit from specifying the types of Numpy arrays unless you're indexing them directly. You may also lost a little speed due to unnecessary type checking. Therefore, for something like main (and other functions here...) where you don't index your arrays directly there is really very little point in specifying the types.

You also never get a lot out of np.ndarray (without specifying the type stored in the array).

Compilation decorators

You applied

@cython.wraparound(False)
@cython.nonecheck(False)
@cython.boundscheck(False)

to every function. These only make a difference for Cython's fast indexing into Numpy arrays (with ints and slices) so for most functions here they will make no difference at all. It is better to think about where you actually need these (and where bounds checking might be helpful, for example) rather than applying these blindly in the hope they'll do something.

There's quite a bit of code here so I'm not going to be comprehensive, but here are some observations.

Profiling

The first thing you should do is to try to profile your code and work out where time is actually being spent. Concentrate your optimization in these functions.

get26Neighbors

I doubt if this function is limiting your speed. However, np.delete is often a little slow since it involves moving elements about in an already allocated array. I'd be tempted to allocate an empty array and fill it instead:

cdef np.ndarray[np.int_t, ndim=2] neighbors = np.empty((26,3),dtype=np.int)
neighbors[:12,0] = xx[:12]
neighbors[14:,0] = xx[14:]
neighbors[:12,1] = yy[:12]
# etc...
# (check my maths on which element to skip)

getNeighborLabels

Indexing using arr[tuple(neighbor_index)] will be slow. In order to take advantage of Cython's ability to index arrays quickly you need to index using only integers and slices, not general Python objects like tuple. It's better to explicitly spell out the indices (event if this is a little more code):

for i in range(neighbors.shape[0]):
    xi = neighbors[i,0]
    yi = neighbors[i,1]
    zi = neighbors[i,2]
    if arr[xi,yi,zi] == 1 and labels[xi,yi,zi] != 0:
        neighborLabels.append(labels[xi,yi,zi])

I've also changed the outer loop to an integer indexed loop since this is much quicker in Cython than using the Python iterator protocol (for x in neighbours) - I know this is generally considered worse style in Python, but it is faster here. There's other places you could apply this.

I'd also be tempted to make neighborLabels a list rather than a numpy array, since Python lists are designed to have quick appends while numpy arrays aren't.

General comments on typing

You've specified explicit types for (almost) everything. You don't generally get much benefit in Cython from specifying basic Python types such as dict or tuple. You also don't benefit from specifying the types of Numpy arrays unless you're indexing them directly. You may also lost a little speed due to unnecessary type checking. Therefore, for something like main (and other functions here...) where you don't index your arrays directly there is really very little point in specifying the types.

You also never get a lot out of np.ndarray (without specifying the type stored in the array).

Compilation decorators

You applied

@cython.wraparound(False)
@cython.nonecheck(False)
@cython.boundscheck(False)

to every function. These only make a difference for Cython's fast indexing into Numpy arrays (with ints and slices) so for most functions here they will make no difference at all. It is better to think about where you actually need these (and where bounds checking might be helpful, for example) rather than applying these blindly in the hope they'll do something.

There's quite a bit of code here so I'm not going to be comprehensive, but here are some observations.

Profiling

The first thing you should do is to try to profile your code and work out where time is actually being spent. Concentrate your optimization in these functions.

get26Neighbors

I doubt if this function is limiting your speed. However, np.delete is often a little slow since it involves moving elements about in an already allocated array. I'd be tempted to allocate an empty array and fill it instead:

cdef np.ndarray[np.int_t, ndim=2] neighbors = np.empty((26,3),dtype=np.int)
neighbors[:12,0] = xx[:12]
neighbors[12:,0] = xx[13:]
neighbors[:12,1] = yy[:12]
# etc...
# (check my maths on which element to skip)

getNeighborLabels

Indexing using arr[tuple(neighbor_index)] will be slow. In order to take advantage of Cython's ability to index arrays quickly you need to index using only integers and slices, not general Python objects like tuple. It's better to explicitly spell out the indices (event if this is a little more code):

for i in range(neighbors.shape[0]):
    xi = neighbors[i,0]
    yi = neighbors[i,1]
    zi = neighbors[i,2]
    if arr[xi,yi,zi] == 1 and labels[xi,yi,zi] != 0:
        neighborLabels.append(labels[xi,yi,zi])

I've also changed the outer loop to an integer indexed loop since this is much quicker in Cython than using the Python iterator protocol (for x in neighbours) - I know this is generally considered worse style in Python, but it is faster here. There's other places you could apply this.

I'd also be tempted to make neighborLabels a list rather than a numpy array, since Python lists are designed to have quick appends while numpy arrays aren't.

General comments on typing

You've specified explicit types for (almost) everything. You don't generally get much benefit in Cython from specifying basic Python types such as dict or tuple. You also don't benefit from specifying the types of Numpy arrays unless you're indexing them directly. You may also lost a little speed due to unnecessary type checking. Therefore, for something like main (and other functions here...) where you don't index your arrays directly there is really very little point in specifying the types.

You also never get a lot out of np.ndarray (without specifying the type stored in the array).

Compilation decorators

You applied

@cython.wraparound(False)
@cython.nonecheck(False)
@cython.boundscheck(False)

to every function. These only make a difference for Cython's fast indexing into Numpy arrays (with ints and slices) so for most functions here they will make no difference at all. It is better to think about where you actually need these (and where bounds checking might be helpful, for example) rather than applying these blindly in the hope they'll do something.

Source Link
DavidW
DavidW
  • 141
  • 5

There's quite a bit of code here so I'm not going to be comprehensive, but here are some observations.

Profiling

The first thing you should do is to try to profile your code and work out where time is actually being spent. Concentrate your optimization in these functions.

get26Neighbors

I doubt if this function is limiting your speed. However, np.delete is often a little slow since it involves moving elements about in an already allocated array. I'd be tempted to allocate an empty array and fill it instead:

cdef np.ndarray[np.int_t, ndim=2] neighbors = np.empty((26,3),dtype=np.int)
neighbors[:12,0] = xx[:12]
neighbors[14:,0] = xx[14:]
neighbors[:12,1] = yy[:12]
# etc...
# (check my maths on which element to skip)

getNeighborLabels

Indexing using arr[tuple(neighbor_index)] will be slow. In order to take advantage of Cython's ability to index arrays quickly you need to index using only integers and slices, not general Python objects like tuple. It's better to explicitly spell out the indices (event if this is a little more code):

for i in range(neighbors.shape[0]):
    xi = neighbors[i,0]
    yi = neighbors[i,1]
    zi = neighbors[i,2]
    if arr[xi,yi,zi] == 1 and labels[xi,yi,zi] != 0:
        neighborLabels.append(labels[xi,yi,zi])

I've also changed the outer loop to an integer indexed loop since this is much quicker in Cython than using the Python iterator protocol (for x in neighbours) - I know this is generally considered worse style in Python, but it is faster here. There's other places you could apply this.

I'd also be tempted to make neighborLabels a list rather than a numpy array, since Python lists are designed to have quick appends while numpy arrays aren't.

General comments on typing

You've specified explicit types for (almost) everything. You don't generally get much benefit in Cython from specifying basic Python types such as dict or tuple. You also don't benefit from specifying the types of Numpy arrays unless you're indexing them directly. You may also lost a little speed due to unnecessary type checking. Therefore, for something like main (and other functions here...) where you don't index your arrays directly there is really very little point in specifying the types.

You also never get a lot out of np.ndarray (without specifying the type stored in the array).

Compilation decorators

You applied

@cython.wraparound(False)
@cython.nonecheck(False)
@cython.boundscheck(False)

to every function. These only make a difference for Cython's fast indexing into Numpy arrays (with ints and slices) so for most functions here they will make no difference at all. It is better to think about where you actually need these (and where bounds checking might be helpful, for example) rather than applying these blindly in the hope they'll do something.