-
Notifications
You must be signed in to change notification settings - Fork 165
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
Add Perona–Malik anisotropic diffusion algorithm #500
Add Perona–Malik anisotropic diffusion algorithm #500
Conversation
@simmplecoder Is this draft or ready for review? For review, I'd suggest you to add tests which reviewers can run. The examples are useful too, but they don't verify results as well as tests. Any particular reason you added the implementation to |
@mloskot , the algorithm itself is ready, e.g. the interface is the one I want. I will deal with the CI failing tonight. About testing: it is a bit complicated. We can do simple sanity check plus some precomputed examples. The source can be matlab scripts presented by the authors, but I'm not sure about the licensing issues. Will it be okay to use the output of it given we do not own matlab commercial license (I have student one) and our users will not have the license? I could search for other sources but the one from paper would be ideal. Also, the performance is faster than what I thought. I believe it is reasonable for a single threaded CPU implementation. About the location: I'm not aware of any other diffusion algorithms that are widely used. Though I believe it is reasonable to put there, will move it tonight. |
Right, so we are getting to the point when our tests need to be based on comparing whole or part of output image against reference image. I knew I should have implemented the PNM ASCII support done #359 long time ago, actually, for #353 during GSoC 2019. Having this, we could simply write algorithms output to PAM file, then we could do simple
Even for single one, I think it's a good idea reflect some categories in structure of the source files. |
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.
First nit-picking review :-)
@mloskot, after working out the equations for tests, I found that not handling the border and corner cases makes this implementation broken for sufficiently high kappa and iteration counts. I will need to handle those cases, but the code will be qutie a bit more brittle, and possibly way larger. My plan is to introduce a function that will handle the edge cases in a transparent way, but that might not be possible. I will report back once I figure it out (no more than one day should be needed). |
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.
@simmplecoder Your last comment suggest this is still work in progress or is it ready for review?
@mloskot , I've been on a search for a bug, and I believe I found it. The issue is that I'm using completely wrong formula there. I thought that the version that I wrote is actually an improved version of the original, but I was wrong. First of all the summation step should be sum between differences of opposing directions. Second, the flux (which is the product in the summation) step is not diffusivity by difference, but rather diffusivity by double difference. I will check this out and report back. |
@mloskot , I'm a bit lost on what people call anisotropic diffusion. I implemented the one that is clearly mentioned in the book, but it still leaks luminosity heavily. I will check out opencv and if it behaves the same way, I will just drop the second test. If my implementation is wrong, I will probably put this on a backburner until the later parts of GSoC. Book I'm using: link. The formula is on page 81 (page 9 of the pdf). I will deal with the CI failures after figure out if my formula is correct. |
@simmplecoder I think OpenCV implementation is based this original paper, I will try to dig this a bit further and look at your code. |
@mloskot , I believe the current version is the correct one according to the paper. I have updated the description though, please have a look. |
c588101
to
da22525
Compare
@simmplecoder The PR is getting close to be merged, would it be possible to ensure the CI builds pass? |
f21242c
to
bee7e82
Compare
@mloskot , I almost finished everything. The only thing left is to cover laplace functions with tests, and adapt demo to include command line argument to choose between default, classic and matlab versions of anisotropic diffusion. Code is well tested. Even the diffusivity functions that are not part of any configuration are tested so that in case somebody uses them they are not broken. Overall this is quite a bit of code, please let me know if some parts don't make sense. I guess I'll need to write up good docs pages for this, though we will need to start from the beginning (topic - finite differences and PDEs). I guess we could do docs at the end of GSoC. |
@simmplecoder First, I'm terribly sorry, for the long silence again. |
The compilation of example will still produce some warnings, but they are unrelated to the new code
There was a bug in nabla computation that relied on gray layout
d3043db
to
f8e382d
Compare
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.
@simmplecoder Excellent work!
This is a complex topic and, I think, you've managed to very well, despite quite a number of hurdles encountered on your way. The interface for the anisotropic diffusion is as simple as possible, yet extensible via pluggable strategies.
I'm happy to accept this PR.
The only aspects that could be improved is adding some code comments that could help a reader trace the calculation process and find references between the implementation and the Perona-Malik paper.
A piece of documentation would be awesome too.
I think, we can assume there are no objections to merging it.
Please, let me know if/when this is ready to merge.
Since this cannot be captured by ref, it is captured by value instead. The change is to not declare redundant variables
I had paranoia about current version not being correct. Texts mention that conductivity in south direction is negative of north. What they don't mention is that they take different cells, the ones shifted by one in south direction. Lets say |
@simmplecoder Good finding. I think I've seen different approaches to orientation/origin of kernel in some papers. |
Also decrease seed range to make test duration manageable.
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.
@simmplecoder Finally, I'm happy to approve this PR
I'm still hoping we will be able to add some piece of documentation for this feature,
but that has to happen after the merge.
Fixes linker error due to multiple definitions. The issue was missed during review of #500 as some public headers are still missing from boost/gil.hpp.
Fixes linker error due to multiple definitions. The issue was missed during review of boostorg#500 as some public headers are still missing from boost/gil.hpp.
The output type must be floating point, thus a check was added to make sure it is the case. Though I had to add specific cases for float32_t as std::is_floating_point does not consider it a floating point type The accumulate part was wrong, it multiplied by delta_t on every sum, which is wrong Use 8 way nabla compute. This is just different discretization of Laplace operator https://en.wikipedia.org/wiki/Discrete_Laplace_operator Laplace stencils are now the same as in mathematical notation The new function will provide a uniform way to generate stencils by making sure directions are indexed properly Add only required stencil points: The 5 points Laplace stencil is now adding only required points and not assuming that others are zero
Fixes linker error due to multiple definitions. The issue was missed during review of boostorg#500 as some public headers are still missing from boost/gil.hpp.
The output type must be floating point, thus a check was added to make sure it is the case. Though I had to add specific cases for float32_t as std::is_floating_point does not consider it a floating point type The accumulate part was wrong, it multiplied by delta_t on every sum, which is wrong Use 8 way nabla compute. This is just different discretization of Laplace operator https://en.wikipedia.org/wiki/Discrete_Laplace_operator Laplace stencils are now the same as in mathematical notation The new function will provide a uniform way to generate stencils by making sure directions are indexed properly Add only required stencil points: The 5 points Laplace stencil is now adding only required points and not assuming that others are zero
Fixes linker error due to multiple definitions. The issue was missed during review of boostorg#500 as some public headers are still missing from boost/gil.hpp.
Description
Implement anisotropic diffusion as defined by Perona-Malik equation.
There are quite a few variations of the solution for the PDE, so I would like to implement the algorithm in the following way:
With that, we can have defines for the known ones:
Then just have
Note that kappa and delta_t (diffusion speed) will be encoded inside the strategy, thus there no need for more arguments. I believe it will be possible to call it like the following:
In the worst case I will just provide an overload.
References
Tasklist