-
Notifications
You must be signed in to change notification settings - Fork 150
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
Fix Laplacian
#586
Fix Laplacian
#586
Conversation
@theotherphil Do you want rename |
I definitely don't agree with the changes you made, especially removing the laplacian_edge_detector function |
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 There are no academic names for the As for the Perhaps those who need the Laplacian 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 |
@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
@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. |
I don't think supporting multiple approximations of the Laplacian is worth it, But we have much bigger problems now, such as 2 incorrect functions with unsound properties in |
I agree with favoring correctness over feature completeness. Removing the |
Ok, let’s fix laplacian_filter for now and worry about getting a correct edge detection algorithm working later. |
To close #585