Re: contributing C/Cython code to the scikit
Hi Ma�l, OK, so I went through almost all your changes. I made some changes myself that you can find on http://github.com/emmanuelle/scikits.image/tree/denoise What I did is basically: * cosmetic changes as described in the PEPs (spaces around operators, 4-space indentation) * docstrings: I modified the examples so that they could work with the Numpy array "lena" provided by Scipy. This avoids I/O and makes the examples easier to understand. * adding a test_denoise.py script in the tests submodule of the filter module. When you write functions that contain the string "test" in their name (e.g. test_tvdenoise), some programs (such as nosetests) will automatically detect and run such tests. Once again, it is better if you can use synthetic data or scipy's lena in these tests to avoid I/O. Further changes that I may suggest: * write a test script filter/tests/test_median.py in the same spirit of filter/tests/test_tvdenoise.py * I think that 'tv_denoise' is easier to read and understand than 'tvdenoise', maybe you should change it. * docstrings: it's nice to provide a wikipedia page and/or the reference to a research article in the "References" part, so that people can easily learn more about the algorithm. It will especially important for the state-of-the-art algorithms that you have in your C library Megawave, and that cannot be found elsewhere. * if there is no good reason to keep lena256.tif (if you can use scipy.lena() in all your examples and tests), then why not remove the file. * in the future, try to make commits that are as unitary as possible (for example, one commit for the tvdenoise algorithm, one for the corresponding test file, then for the median filter, etc.). Of course, it's important that the commit does not break anything (build, etc.). I did not have the time to read thoroughly the code of your median filter, I think you wanted to know if this was efficient Cython code or not. I'll do it soon, maybe after you write a test function for this filter? By the way, what if the advantage of this function compared to scipy.ndimage.median_filter? I also haven't gone though the C code yet. You can check PEP 7 to see if the C code also matches the coding guidelines. I have to say that I'm not a very experienced programmer either (at least for large or medium-size collaborative projects), so I would be happy if someone corrects me if I'm wrong on some points. Cheers, Emmanuelle On Mon, Sep 20, 2010 at 10:02:16AM -0700, mael wrote:
Hello everyone,
here is a first code draft, including two basic filters http://github.com/maelp/scikits.image
the goal is not to include high-profile algorithms right now, but to get the base framework correctly (and then we'll have some engineer translate our codebase to python). could you tell me if the way I started it seems okay with you? (eg I've been hacking in some files to have everything compile.. am I doing it as it should be done?)
feel free to comment on code and possible issues , as we're trying to have some solid base to build upon :)
Thanks Emmanuelle for your code refactoring and the remarks,
* if there is no good reason to keep lena256.tif (if you can use scipy.lena() in all your examples and tests), then why not remove the file. sure
I did not have the time to read thoroughly the code of your median filter, I think you wanted to know if this was efficient Cython code or not. I'll do it soon, maybe after you write a test function for this filter? By the way, what if the advantage of this function compared to scipy.ndimage.median_filter? No there is no reason to use our own rather than scipy's one (except that my implementation might be less obfuscated because it is less complicated) it was more an example of what we could do to bind C and python
On Tue, Sep 21, 2010 at 9:22 PM, Emmanuelle Gouillart <emmanuelle.gouillart@normalesup.org> wrote:
* docstrings: I modified the examples so that they could work with the Numpy array "lena" provided by Scipy. This avoids I/O and makes the examples easier to understand.
Also note that we ship a small collection of test images with the scikit. These may be found in scikits.image.data_dir Regards Stéfan
participants (3)
-
Emmanuelle Gouillart
-
mael
-
Stéfan van der Walt