<br><br><div class="gmail_quote">On Sat, Dec 31, 2011 at 6:51 AM, Tony Yu <span dir="ltr"><<a href="mailto:tsyu80@gmail.com">tsyu80@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br><div class="gmail_quote"><div class="im">On Fri, Dec 30, 2011 at 5:48 PM, Ralf Gommers <span dir="ltr"><<a href="mailto:ralf.gommers@googlemail.com" target="_blank">ralf.gommers@googlemail.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br><br><div class="gmail_quote"><div>On Fri, Dec 30, 2011 at 8:05 PM, Tony Yu <span dir="ltr"><<a href="mailto:tsyu80@gmail.com" target="_blank">tsyu80@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div><br><br><div class="gmail_quote">On Fri, Dec 30, 2011 at 1:19 PM, Ralf Gommers <span dir="ltr"><<a href="mailto:ralf.gommers@googlemail.com" target="_blank">ralf.gommers@googlemail.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br><br><div class="gmail_quote"><div>On Fri, Dec 30, 2011 at 5:46 PM, Tony Yu <span dir="ltr"><<a href="mailto:tsyu80@gmail.com" target="_blank">tsyu80@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br><br><div class="gmail_quote">On Fri, Dec 30, 2011 at 8:20 AM, Nadav Horesh <span dir="ltr"><<a href="mailto:nadavh.horesh@gmail.com" target="_blank">nadavh.horesh@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div dir="ltr">Remarks for the convolution subpackage:<div>1. The __init__.py should be probably changed to</div><div><br></div><div>from ext import pyconvolve as convolve</div></div></blockquote><div><br>Agreed. Alternatively, the c-method could be renamed so that the python method in ext can be named convolve.<br>
</div><blockquote class="gmail_quote" style="margin:0pt 0pt 0pt 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div><br></div><div>2. 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.<br>
</div></div></blockquote><blockquote class="gmail_quote" style="margin:0pt 0pt 0pt 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr">
<div><br></div><div>3. I found the convolve speed in par with cv.convolve2d (it can be slower or faster depending on the kernel)</div><span><font color="#888888"><div><br></div><div> Nadav</div></font></span></div>
</blockquote><div><br>Thanks for testing the code.<br><br>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?<br>
<br>-Tony<br><br>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:<br>
<br>export ARCH_FLAGS="-arch i386 -arch x86_64"<br></div></div></blockquote></div><div><br>Can you explain why is this necessary? <br><br>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.<span><font color="#888888"><br>
<br>Ralf</font></span><br></div></div></blockquote></div><br></div>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.<br>
</blockquote></div><div><br>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?<br>
</div></div></blockquote></div><div><br>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 <a href="http://en.wikipedia.org/wiki/PowerPC" target="_blank">Wikipedia page</a>, it seems like PPC is mainly used in servers now.<br>
</div></div></blockquote><div><br>OS X on older PPC machines is still common enough, for example the laptop I build numpy/scipy binaries on has a PPC cpu. Also, even for many Intel based machines the Python defaults include PPC compilation, so this is going to trip people up. Therefore a fallback would be useful.<br>
<br>Ralf<br><br><br></div><blockquote class="gmail_quote" style="margin:0pt 0pt 0pt 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div class="gmail_quote"><div>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.<br>
<br>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.<span class="HOEnZb"><font color="#888888"><br>
<br>-Tony<br><br></font></span></div><div class="im"><blockquote class="gmail_quote" style="margin:0pt 0pt 0pt 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div class="gmail_quote"><div><blockquote class="gmail_quote" style="margin:0pt 0pt 0pt 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>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.<span><font color="#888888"><br>
</font></span></blockquote></div><div><br>It's very likely to give problems with unusual (or even Intel) compilers I think.<span><font color="#888888"><br><br>Ralf<br> <br></font></span></div></div>
</blockquote></div></div><br>
</blockquote></div><br>