Hi, Please see inline comments. 2009/10/11 Stéfan van der Walt <stefan@sun.ac.za>:

Hi Chris

2009/10/10 Chris Colbert <sccolbert@gmail.com>:

...

I've made a fork on github to work on the opencv stuff:

http://github.com/sccolbert/scikits.image

I've also made a pull request to Stefan so the initial stuff will be available in the trunk.

Thanks, things are progressing really well!

As always, I'd like to do a round of review before integrating the code.  Standard disclaimer: my comments are just my opinions, so feel free to say if you disagree.  OK, here goes:

- Instead of trying to load a .dll or .so yourself, make use of numpy's np.ctypeslib.load_library.  If might also be a good time to document that function (please!).  For example, see line 16 of

http://dip.sun.ac.za/~stefan/code/supreme.git/?p=stefan/supreme.git;a=blob;f...

I see that function is completely undocumented, is it safe to assume it handles the loading of the library regardless of the platform, making my checks for windows and linux unecessary?

- I saw the comment that you'd like to find a better way to match types in numpy and openCV, but really I think your dictionary is just fine.

I wasn't quite sure if the performance of a dictionary lookup would be good enough for our case. And I thought that may there was some basic computer science concept that I was missing that would be implemented there (I'm a mechanical engineer and self taught programmer). But if you say the dictionary is fine for that purpose, that suits me just as well.

- Just to clarify things in my own mind: In opencv_type.pxd, you say that you reimplement the _IplImage type.  Is this basically a direct copy from the IplImage.h header file?  I guess you had to redefine ROI and maskROI to point to void?

Yes, it's basically a direct copy from the opencv header file, with a few things set to void that will never be used. I though this would be cleaner than using a "cdef extern from" declaration to get the type from the opencv headers. Plus, we the don't need the opencv headers present on the client machine during build time.

I'd be very happy to integrate this branch as soon as there are some tests (to make sure the round-tripping of ndarray -> IplImage -> ndarray works, for example).  You'll have to make sure that the tests are marked for skipping if opencv is not available.  Also, could you document the OpenCV wrappers (like cvSmooth) to have a pointer to the online OpenCV docs?  That would help users to get up and running quickly!

Yes, I need to go back and add some docstrings, definately. Tests, I was a little unsure of how you want to make the tests. Do you want to test everypossible combination of every function? or just one test case on a small array, and verify the output is correct?

Finally, we'll have to figure out a way to load libopencv.so even if it is installed in standard locations (e.g., on the different Linux distros such as Debian).

the library will load provided the directory it is in is available on LD_LIBRARY_PATH in linux, or the equivalent path in windows. I think it should be the responsibility of the user to make sure their libraries are available on the path, instead of us trying to hunt them down.

Keep up the good work!

Cheers Stéfan

Cheers, Chris

After taking a look at numpy.ctypeslib.load_library(), is there any particular reason to use that over ctypes.CDLL(). I find the advantage goes to the latter. With the numpy function, you have to specify the path where the library to be loaded exists, using ctypes.CDLL() the library just has to be available on the system library path. The only advantage I see to the numpy method is that is takes care of the library file extension for you (something we could easily do ourselves), but at the expense of having to try to find the libraries ourselves rather than letting the OS take care of it. Unless there is strong backlash, I would much rather use ctypes.CDLL() to load the opencv libs... Cheers, Chris 2009/10/11 Stéfan van der Walt <stefan@sun.ac.za>:

2009/10/11 Chris Colbert <sccolbert@gmail.com>:

...

...

- Instead of trying to load a .dll or .so yourself, make use of numpy's np.ctypeslib.load_library.  If might also be a good time to document that function (please!).  For example, see line 16 of

http://dip.sun.ac.za/~stefan/code/supreme.git/?p=stefan/supreme.git;a=blob;f...

I see that function is completely undocumented, is it safe to assume it handles the loading of the library regardless of the platform, making my checks for windows and linux unecessary?

Yes, that's correct..  If you don't mind, it would be great if you could sign on to http://docs.scipy.org and fix the lack of docs!

...

...

- I saw the comment that you'd like to find a better way to match types in numpy and openCV, but really I think your dictionary is just fine.

I wasn't quite sure if the performance of a dictionary lookup would be good enough for our case. And I thought that may there was some basic computer science concept that I was missing that would be implemented there (I'm a mechanical engineer and self taught programmer). But if you say the dictionary is fine for that purpose, that suits me just as well.

First, you did a good job and, second, the algorithms typically run much longer than the time required to call them.  We'll optimise if the need arises!

...

Tests, I was a little unsure of how you want to make the tests. Do you want to test everypossible combination of every function? or just one test case on a small array, and verify the output is correct?

Yes, I don't think we need to rewrite OpenCV's test suite.  Just a simple test for each function to make sure parameters are passed correctly with the right types and that the output is sane.  You can use the test decorators (have a look in the scipy source) to skip tests when OpenCV is not available.

Cheers Stéfan