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