8000 Small bug in skimage.feature.peak_local_max() · Issue #3019 · scikit-image/scikit-image · GitHub
[go: up one dir, main page]
Skip to content

Small bug in skimage.feature.peak_local_max() #3019

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.

Sign up for GitHub

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

Jump to bottom
Closed
arares opened this issue Apr 16, 2018 · 6 comments
Closed

Small bug in skimage.feature.peak_local_max() #3019

arares opened this issue Apr 16, 2018 · 6 comments

Comments

@arares
Copy link
arares commented Apr 16, 2018

Description

When provided both min_distance and threshold_abs parameters to the skimage.feature.peak_local_max() function, the first parameter takes unnecessary precedence over the second one. I did not look at the code, but from my experiments it appears that a list of peaks is calculated internally, then the first constrained is applied, and then the second one. However, if there is a peak above the given threshold that lies to the right of another one under the threshold, but close to it (under min_distance), it will be rejected because of its proximity. As such, a tiny peak that will be excluded from the final list anyway manages to eclipse a big peak. I think the threshold criterion should be applied before the minimal distance criterion. Even better, the problem would not appear if the list of thresholds would be processed in decreasing height order, rather than top-down, left-right order (as I guess it is done now).
@sciunto
Copy link
Member
sciunto commented Apr 24, 2018

Hi,

Thanks for the report.
Your proposition makes sense to me. However, we are planning to refactor the peak detection provided in scikit-image (See #3022) and it might not be worth to fix a function that will be deprecated relatively soon.

@emmanuelle
Copy link
Member

Thank you for the issue. We need to fix it. Can you please provide a small snippet of code so that we can reproduce the problem (and put it in a test for the future function)?

@stefanv
Copy link
Member
stefanv commented Jun 1, 2018

This seems to be orthogonal to the changes proposed in #3022.

Solution: sort peaks by amplitude, and suppress nearby neighbors.

@sciunto
Copy link
Member
sciunto commented Jun 2, 2018

I thought that #3022 would have been merged in 0.14. Now it makes sense to merge a fix in the LTS version.

@jni
Copy link
Member
jni commented Jun 2, 2018

#3022 is almost definitely destined for 0.14 as well.

< 86E7 /div>
@rfezzani
Copy link
Member

I think this is fixed now by #4501 and #4582. Feel free to reopen if you disagree 😉

@alexdesiqueira alexdesiqueira mentioned this issue Oct 13, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
7 participants
@stefanv @emmanuelle @sciunto @jni @soupault @rfezzani @arares
0