Skip to content
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

Use Euclidean distance in peak_local_max as new default #6608

Open
patquem opened this issue Nov 3, 2022 · 9 comments
Open

Use Euclidean distance in peak_local_max as new default #6608

patquem opened this issue Nov 3, 2022 · 9 comments

Comments

@patquem
Copy link
Contributor

patquem commented Nov 3, 2022

Description:

Hi,
It took me a while to understand why some maximum points were not detected in my case (diffraction images of a crystal).
I finally found the reason: the default choice for the distance calculation with p-norm=np.inf.
My personal representation of a distance in space tends naturally towards Euclidean distance.
So why such a choice (p_norm = inf) ? p-norm=2 wouldn't be more appropriate ?
Patrick

@stefanv
Copy link
Member

stefanv commented Nov 3, 2022

With p_norm=inf you get the Chebychev distance, which can be read "largest difference along any axis". I don't know why that was chosen as the default. Perhaps @rfezzani, who implemented it, can comment?

@rfezzani
Copy link
Member

rfezzani commented Nov 4, 2022

Hello @patquem, and sorry for the late answer 🙏.
This choice was made for retro-compatibility reason! Previous implementation was searching for the local minima in square region around each localization, which corresponds to Chebychev distance...
@scikit-image/core, maybe this default choice can revised for the future major version?

@patquem
Copy link
Contributor Author

patquem commented Nov 7, 2022

Hello,
Thank you for your answers.
I understand the reason of retro-compatibility.
Hope you can revisit this :)

Below is an illustration of the confusion that the current choice can cause.
The crosses in the squares are the Maxima detected with p_norm=np.inf.
In the second figure, only 1 maximum is detected while the 2nd spot is further away than in the first figure (sqrt(50)~7 px vs 6 px)

image

import numpy as np
import matplotlib.pyplot as plt
from skimage.feature import peak_local_max

arr0 = np.zeros((10, 10))
arr1 = np.zeros((10, 10))

arr0[2, 2] = 1
arr0[2, 8] = 1

arr1[2, 2] = 1
arr1[7, 7] = 1


def plot(ax, arr):
    coords = peak_local_max(arr,
                            min_distance=6,
                            exclude_border=False)
    ax.imshow(arr, origin='lower')
    for i, (y, x) in enumerate(coords):
        ax.plot(x, y, 'rx', ms=10, markerfacecolor="None")


fig, (ax0, ax1) = plt.subplots(1, 2)
plot(ax0, arr0)
plot(ax1, arr1)
plt.show()

@lagru
Copy link
Member

lagru commented Nov 7, 2022

I'd also advocate p_norm=2 / Euclidean distance as the least surprising default here.

https://github.com/orgs/scikit-image/teams/core, maybe this default choice can revised for the future major version?

That would probably be easiest. Alternative would be a deprecation, perhaps using a sentinel value to signal a changing default. But tagging this for the v2 API for now, so we don't forget it.

@lagru lagru added this to the skimage2 milestone Nov 7, 2022
@lagru lagru changed the title min_distance in peak_local_max : p_norm = 2 as default instead of p_norm = np.inf ? Use Euclidean distance in peak_local_max as new default Nov 7, 2022
@lagru
Copy link
Member

lagru commented Nov 7, 2022

@patquem I took the liberty to make the title a bit more concise but that may be subjective. 😉

@rfezzani
Copy link
Member

rfezzani commented Nov 7, 2022

@patquem

... while the 2nd spot is further away than in the first figure (sqrt(50)~7 px vs 6 px)

This is true if you consider the Euclidean 2-norm, but with the $\infty$-norm the corresponding distances are respectively 5 vs 6 😉.

@github-actions
Copy link

github-actions bot commented May 7, 2023

Hey, there hasn't been any activity on this issue for more than 180 days. For now, we have marked it as "dormant" until there is some new activity. You are welcome to reach out to people by mentioning them here or on our forum if you need more feedback! If you think that this issue is no longer relevant, you may close it by yourself; otherwise, we may do it at some point (either way, it will be done manually). In any case, thank you for your contributions so far!

@github-actions github-actions bot added the 😴 Dormant no recent activity label May 7, 2023
@lagru
Copy link
Member

lagru commented May 7, 2023

I added it to our informal todo list for skimage2 so it isn't forgotten. 😉

@lagru
Copy link
Member

lagru commented May 7, 2023

Note that I moved the list to our Wiki https://github.com/scikit-image/scikit-image/wiki/API-changes-for-skimage2.

@github-actions github-actions bot removed the 😴 Dormant no recent activity label May 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants