Fix `Laplacian`

[cospectrum]

To close https://github.com/image-rs/imageproc/issues/585

[cospectrum]

@theotherphil Do you want rename laplacian_filter to laplacian?

[thecodergus]

I definitely don't agree with the changes you made, especially removing the laplacian_edge_detector function

[theotherphil]

laplacian_filter is consistent with the other filters in this file so let’s stick with that. (This function maybe belongs with the filters in gradients.rs but it’s not a gradient so neither place is obviously better.)

Changing the return type to i16 is definitely correct - I should have spotted that before merging.

Changing the kernel used is more a matter of preference - both the existing kernel and the one in your PR are commonly used approximations to the Laplacian. I’d add an enum to this function letting you choose which to use; we just need to come up with sensible names for the two options!

Deleting the edge detector isn’t necessary - the output from the Laplacian filter can be scaled to a u8 value range before thresholding. @thecodergus where does the use of Otsu thresholding for this come from? Presumably they use absolute values of intensity rather than linear scaling before applying the threshold?

[cospectrum]

@theotherphil There are no academic names for the Laplacian approximation. And I don't think we can come up with good names.

As for the detector, I don't think I can fix it with scaling :). I think otsu threshold is not the right tool for the job. We should simply search for ≈zero points after the Laplacian. Also approximation of f(x) = laplacian(gaussian(x)) probably should be better than gaussian followed by laplacian in speed and accuracy.

Perhaps those who need the Laplacian detector in the master will reimplement it in another PR with some math proofs behind it? I just don't like that we have a broken master. It makes me very sad.

And in general, I would prefer to have fewer functions, but so that they are 100% correct, than to have a bunch of functions that only partially work. This is the main reason why I use imageproc instead of opencv. Users are not stupid, imageproc already provides enough building blocks to implement the laplacian of gaussian or whatever in 2 lines.

[thecodergus]

laplacian_filter is consistent with the other filters in this file so let’s stick with that. (This function maybe belongs with the filters in gradients.rs but it’s not a gradient so neither place is obviously better.)

Changing the return type to i16 is definitely correct - I should have spotted that before merging.

Changing the kernel used is more a matter of preference - both the existing kernel and the one in your PR are commonly used approximations to the Laplacian. I’d add an enum to this function letting you choose which to use; we just need to come up with sensible names for the two options!

Deleting the edge detector isn’t necessary - the output from the Laplacian filter can be scaled to a u8 value range before thresholding. @thecodergus where does the use of Otsu thresholding for this come from? Presumably they use absolute values of intensity rather than linear scaling before applying the threshold?

@theotherphil, I agree that using enums is an excellent idea to allow the user to choose between the two variants of the Laplacian. As for the use of Otsu thresholding in the edge detector, it derives from my empirical experience in research and innovation projects. Its purpose is to highlight the edges more visibly in the final image, and although there is no specific theoretical reason to use the Otsu technique, tests over the years have demonstrated its effectiveness. Regarding the idea of using absolute intensity values instead of linear scaling before threshold application, it is certainly something to be considered

@theotherphil There are no academic names for the Laplacian approximation. And I don't think we can come up with good names.

As for the detector, I don't think I can fix it with scaling :). I think otsu threshold is not the right tool for the job. We should simply search for ≈zero points after the Laplacian. Also approximation of f(x) = laplacian(gaussian(x)) probably should be better than gaussian followed by laplacian in speed and accuracy.

Perhaps those who need the Laplacian detector in the master will reimplement it in another PR with some math proofs behind it? I just don't like that we have a broken master. It makes me very sad.

And in general, I would prefer to have fewer functions, but so that they are 100% correct, than to have a bunch of functions that only partially work. This is the main reason why I use imageproc instead of opencv. Users are not stupid, imageproc already provides enough building blocks to implement the laplacian of gaussian or whatever in 2 lines.

@cospectrum, I am confident that we can find good names for the Laplacian variants. Over the weekend, I will review the literature to explore this idea and also consult my professor, whose primary research area is image processing. Thus, I hope we can solve this problem and keep the master branch functioning correctly. Finally, never underestimate the user's capacity. My goal in creating the edge detection function is to provide a complete and well-structured solution so that the user does not have to resort to auxiliary functions or possess specific academic knowledge to use it successfully.

[cospectrum]

I don't think supporting multiple approximations of the Laplacian is worth it, enum means something finite and different, but there are an infinite number of approximations that logically perform the same task, we will not cover all of them anyway. And it will make usage annoying. If the user needs something specific, he can already call filter3x3 in 1 line.

But we have much bigger problems now, such as 2 incorrect functions with unsound properties in master. And I'm sure there are production codebases that depend on master, that's why I am here

[ripytide]

I agree with favoring correctness over feature completeness. Removing the laplacian_edge_detector() function with it's rather arbitrary implementation for now seems like the best way forward as we can always add it back later after it's implementation details have been justified a bit more thoroughly. imageproc should try and stay as unopinionated as possible for the greatest flexibility.

[theotherphil]

Ok, let’s fix laplacian_filter for now and worry about getting a correct edge detection algorithm working later.