contributing C/Cython code to the scikit

Emmanuelle Gouillart emmanuelle.gouillart at normalesup.org
Tue Sep 21 15:22:11 EDT 2010


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 :)



More information about the scikit-image mailing list