On Fri, Dec 30, 2011 at 5:48 PM, Ralf Gommers firstname.lastname@example.org:
On Fri, Dec 30, 2011 at 8:05 PM, Tony Yu email@example.com wrote:
On Fri, Dec 30, 2011 at 1:19 PM, Ralf Gommers < firstname.lastname@example.org> wrote:
On Fri, Dec 30, 2011 at 5:46 PM, Tony Yu email@example.com wrote:
On Fri, Dec 30, 2011 at 8:20 AM, Nadav Horesh firstname.lastname@example.org:
Remarks for the convolution subpackage:
- The __init__.py should be probably changed to
from ext import pyconvolve as convolve
Agreed. Alternatively, the c-method could be renamed so that the python method in ext can be named convolve.
- The test_convolution.py file is very instructive. It should be
polished a bit. I think it would be worthwhile to add also scipy.signal.sepfir2d and cv2.sepFilter2D to the test.
- I found the convolve speed in par with cv.convolve2d (it can be
slower or faster depending on the kernel)
Thanks for testing the code.
I've got the code compiling on my system now, so I was able to clean up a few things. Could you update to what's currently on my skimage-convolution branch to check that it builds on your system? If not, what flags do you have to change/add to get it to compile?
P.S. If any one else is building this on OS X: ppc compatible builds is turned on by default (at least on my system). This should be turned off before building. Add the following line to ~/.profile, or just run it right before building:
export ARCH_FLAGS="-arch i386 -arch x86_64"
Can you explain why is this necessary?
In convolution/setup.py I also see you add "-ffast-math", which is a GCC-specific flag. It's better to remove it I think, it may give problems with other compilers.
On my system at least, running gcc will, by default, add "-arch ppc" along with i386 and x86_64. As far as I can tell, PPC doesn't support SSE, which is used by this implementation.
That's true. I can't tell for sure, but it seems that this code is not optional. Meaning PPC is unsupported, and the ARCH_FLAGS need to be defined on OS X. I'm only a very occasional user now so it doesn't bother me too much, but isn't this a large penalty for the benefit you get?
Well, I assume that we would be able to add a check to disable the build on unsupported architectures. As far as, whether it's worth it: I don't know. Is PPC very common? I'm only familiar with it in the context of old Apple computers. From the Wikipedia page http://en.wikipedia.org/wiki/PowerPC, it seems like PPC is mainly used in servers now.
As for the benefit: I think getting close to the performance of OpenCV is pretty significant. (On my system, however, I don't quite see the performance boost that Nadav sees: skimage: 0.034 secs, ndimage: 0.089 secs, opencv: 0.018 secs, for the output of 'test_convolution.py'---nevertheless, it's a significant improvement over ndimage). And, in comparison, OpenCV can be incredibly difficult to build.
There are probably other ways to get similar performance, but like I said, I don't do any serious non-Python coding, so I have no idea what those alternatives may be. Stefan and Pieter (the original author of this code) have probably thought this through a bit more.
The flag is left over from the original implementation. Removing "-ffast-math" works fine on my system. To be honest, I don't do much non-Python coding so I don't know the implications of adding/removing the flag.
It's very likely to give problems with unusual (or even Intel) compilers I think.