-
-
Notifications
You must be signed in to change notification settings - Fork 2.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Rank filters #348
Rank filters #348
Conversation
Great. A contribution arising from the EuroScipy 2012 conference and a meeting between Olivier Debeir and Stefan van der Walt. Thanks Olivier for this contribution. |
@odebeir This is such a good surprize! I am very glad that you had the time to update your code for inclusion in scikit-image. We'll review it in the coming days, and give feedback where applicable. Would you mind if we move your name to the CONTRIBUTORS.txt file? That helps us to have a single license file for the entire project. Also, we'll probably move the skimage.rank module to skimage.filter.rank, and expose the functions as skimage.filter.rank_* or similar. I'm looking forward to getting this integrated over the next week or two! |
2012/10/8 Stefan van der Walt notifications@github.com
Nicolas Pettiaux, dr. sc - gsm : +32 496 24 55 01 |
Hi Stefan, kind regards, Olivier On Mon, Oct 8, 2012 at 10:06 AM, Stefan van der Walt <
|
Hi Olivier, I had a quick look at this branch - this is definitely a great contribution. I especially like the vast performance improvement. Do you have an accessible paper that describes the underlying algorithm? This would make the review process a lot easier. Thank you so far! |
|
||
Contact | ||
------- | ||
Stefan van der Walt <stefan at sun.ac.za> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Strange I haven't seen this yet, why is this being removed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It came in via your PR, I believe.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, I see. And the original change came from you.
I think we are getting close to finally merge this PR. @odebeir I think we should make a feature freeze here and only improve the current implementation. If you have any other features, you'd like to add it's probably best to file a new PR after we this one is actually merged. In my opinion, these are the last things that need to be done:
|
@ahojnnes Ok, no new feature for the moment, thanks for helping me on this |
|
||
def test_otsu(): | ||
# | ||
test = np.tile([128, 145, 103, 127, 165, 83, 127, 185, 63, 127, 205, 43, 127, 225, 23, 127],(16,1)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wrap lines in here and PEP8. Sorry for being so pedantic about this...
autopep is not aware of Cython syntax, |
|
||
|
||
def test_otsu(): | ||
# |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
?
Fantastic work, thanks! Unless there's anything else, I think we're ready to merge. |
Yes, I'm very happy we are finally done with this massive PR. This has been a lot of work! Special thanks @odebeir. |
Indeed! @odebeir next time we meet, your beer's on me! |
@stefanv this brings me to the question: when is the next scipy or whatever conference where the scikit-image team meets? I'm definitely planning to join the next event if possible. |
@ahojnnes Good question. I'm not sure I'll make it to the next US conference, but I'm going to try my best to attend EuroSciPy 2013. But perhaps a significant portion of the scikit-image team will be in South Africa soon hint hint ;) |
Finally merged. |
Good to know. EuroScipy is also the most convenient for me. Concerning South Africa: I'll let you know as soon as I know more or in case I have questions (but the info website served me well so far). |
Hi,
this is my first attempt to contribute to such a collaborative work, so please consider that this is a humble and presumably clumsy contribution,
I am ready to learn, so please do not hesitate to give your thought.
Basically this contribution is an ensemble of filters that compute local histogram for each pixel. Histogram is build using a moving window in order to limit redundant computation.
The path followed by the moving window is given hereunder
...-----------------------
/--------------------------/
-------------------------- ...
A comparison is proposed with cmorph.dilate algorithm to show how computation costs evolve with respect to image size or
structuring element size. This implementation gives better results for large structuring elements.
A local histogram is update at each pixel by introducing pixel entering the structuring element border and by removing those leaving it. The histogram size is 8bit (256 bins) for 8 bit images and 2 to 12 bit (up to 4096 bins) for 16bit image depending on the image maximum value. Image with pixels higher than 4095 raise a ValueError.
The filter is applied up to the image border, the neighboorhood used is adjusted accordingly. The user may provide a mask image (same size as input image) where non zero value are the part of the image participating the the histogram computation. By default all the image is filtered.
Some filters have also a 'soft' version where percentile are used instead of min/max.
A variation on the same algorithm allows to generate a sort of 'bilateral' filter by defining a spectral interval around the current point.
Some example are derived from existing ones. Tests are not exhaustive, I am working on it.
Thank you for feedback
Olivier