Thank you @mkcor for your review ;)

Can we plan this PR for 0.18 or is the schedule too short?

@jni, here is the error reported by @emmanuelle: in corner_peaks doctest we have

>>> from skimage.feature import peak_local_max
>>> response = np.zeros((5, 5))
>>> response[2:4, 2:4] = 1
>>> response
array([[0., 0., 0., 0., 0.],
       [0., 0., 0., 0., 0.],
       [0., 0., 1., 1., 0.],
       [0., 0., 1., 1., 0.],
       [0., 0., 0., 0., 0.]])
>>> peak_local_max(response)
array([[2, 2]])

but with the last fix

>>> peak_local_max(response)
array([[2, 2],
       [2, 3],
       [3, 2],
       [3, 3]])

that's the result with min_distance=1 which is equivalent to min_distance=0!!!

I think that in this case, peak_local_max(response, min_distance=1) must be equal to array([[2, 2]]). Am I wrong?

This is again a pixel centred VS grid centred point of view problem I think!

This is again a pixel centred VS grid centred point of view problem I think!

I don't think so? In either case the distance to the adjacent pixels is exactly 1 or sqrt(2)?

I think the new result is what I would expect. It's unclear to me why it wasn't that before. OR, if you want, change it to use min_distance=2 and the problem should be solved! =)

This is again a pixel centred VS grid centred point of view problem I think!

I don't think so? In either case the distance to the adjacent pixels is exactly 1 or sqrt(2)?

I say that in the sens that pixel (0, 1) is adjacent to pixel (0, 2) (the distance between them is 0 because they join at (0, 0.5)). But the points (0, 1) is separated from the point (0, 2) by a distance of 1.

In other words, do we consider that a pixel is a point or "a little square"? I know, pixels are not squares, but it can be seen as such 😄 (grid centred VS cell centred)

BTW, corner_peaks returns array([[2, 2]]) on this example, and this PR mimic the strategy implemented in corner_peaks.
A wanted to propose a PR to deprecate corner_peaks once this one is merged...

I say that in the sens that pixel (0, 1) is adjacent to pixel (0, 2) (the distance between them is 0 because they join at (0, 0.5)). But the points (0, 1) is separated from the point (0, 2) by a distance of 1.

Well, I think this is a can of worms that we do not want to open, since this definition no longer satisfies the definition of a metric between pixels.

What do you think of my proposal to add min_distance=2 to the example so we can merge this? ;)

A wanted to propose a PR to deprecate corner_peaks once this one is merged...

I'd be into that, I was always confused by that function. But probably for 0.19 as that is a major API change.

OK, let's keep the old behavior considering that consecutive pixels are separated by a distance of 1 for now 😉.

I'd be into that, I was always confused by that function. But probably for 0.19 as that is a major API change.

Let's go for that too, but we will then need to decide which behavior to keep (peak_local_max or corner_peaks)

Ok, as far as I can tell, the failures have nothing to do with this PR. @scikit-image/core anyone else ready to push the green button?

Thank you @grlee77, I added the changes to the release note as you advised 😉.

🎉 thank you all for your reviews! I am happy to see this in 0.18 😄

@rfezzani Just wanted to give you a personal shout-out: the peak finder was performing poorly, and when I looked into what should be done to fixed it I discovered you had already done the work. Thank you very much!