Turn exclude_border off in peak_local_max by default #2447
Conversation
Current coverage is 90.69% (diff: 100%)@@ master #2447 diff @@
==========================================
Files 304 304
Lines 21427 21427
Methods 0 0
Messages 0 0
Branches 1838 1838
==========================================
Hits 19433 19433
Misses 1654 1654
Partials 340 340
|
| @@ -48,9 +48,10 @@ def peak_local_max(image, min_distance=1, threshold_abs=None, | |||
| the minimum intensity of the image. | |||
| threshold_rel : float, optional | |||
| Minimum intensity of peaks, calculated as `max(image) * threshold_rel`. | |||
| exclude_border : int, optional | |||
| exclude_border : int | bool, optional | |||
There was a problem hiding this comment.
int | bool -> int or bool (see https://github.com/numpy/numpy/blob/master/doc/HOWTO_DOCUMENT.rst.txt)
|
Shouldn't we follow a deprecation path? |
|
I don't think so. First, there is nothing going to be removed nor added in place of it. Second, the change of the default value only might cause more peaks popping up for unaware users, which is most certainly better than the other way around. So I think an appropriate remark in the change log is sufficient. |
|
I'm hesitant. At least, I would not include this patch in 0.13 to have enough time to get feedbacks from users who test the dev version. |
|
Uuuuh, other @scikit-image/core devs can chime in, but I'm pretty sure this requires a deprecation path. This is required whenever the output of a function will change for the exact same input. When people update their environment, they should not be left to wonder which of the updates changed their results (NumPy, SciPy, pandas, scikit-image could all be culprits in a complex pipeline). |
|
@jni It's not the exact same input, since the default value changed and thus a different input. Nonetheless, how should one proceed in case a deprecation path is required? |
|
For the record, when I first combined two separate peak-finding functions into what is now What's changed in 4 years? Nothing, except we've led a lot of users astray who had peak(s) near the edge of their images and didn't change the default. Worse, our default links the blind edge to At issue, really, is this: If a function saying "hey I find peaks!" which actually ignores any peaks in a large subset of the image with default settings hostile to users? I'd argue yes. Further, I'd argue that anyone who would be tripped up by changing this default in the vast majority of cases was assuming different behavior than what they got. Users who explicitly set this this Coming at this from another angle - can anyone think of a single common use case where the border exclusion would actually be desired? I can make up very niche reasons this would be nice, but none of them are good reasons. And all of them could be handled by doing peak finding on a sliced down input, e.g., passing In summary, I think this is user hostile behavior and a bug. It should be fixed. There's no easy deprecation pathway, so a documented breaking change is warranted. Frankly, I think removing the setting entirely would be reasonable. |
|
That makes me wonder why it was included in the first place @JDWarner? If there were no legit reasons or use cases, it seems kinda odd to add a feature just for the feature. |
|
@b52 Can't say; I'm unsure of the history before that point, as harmonizing the peak finding was one of my earliest contributions. Maybe there's a decent reason for it I'm missing entirely. |
|
How about: removing the setting and raising a warning for two releases, unless it is specified and set to False. |
|
@stefanv et al Actually I suggest closing this PR. Some points:
The point I'm trying to illustrate is that one cannot be sure whether a maximum on the border is actually a local maximum or rather a truncated upward slope. In fact, the second option is much more probable if you are sampling peaks that are wider than three pixels. So if you don't exclude the borders, you will mostly get junk on the borders, in proportion to how wide your peaks are. And @JDWarner, truncating the image does not actually alleviate this problem because then you will get junk on the inner border — and real peaks in the intervening space will be mis-localized! In other examples, such as object counting that correctly accounts for edge effects, you do want peaks on the borders, but only half the borders.
To summarize my view: a) the option should be maintained going forward @b52 thanks for your PR, and I apologize if I sound rude above... I just want to make my case clear and also not send you down the rabbit hole of deprecation procedures when the function itself might be deprecated soon. |
|
My thinking was rather that we'd introduce another function to filter
coordinates, but I don't have strong feelings on the topic. If we do allow
border position filtering, we could think of a more nuanced way of
specifying values: % of image size, pixels, etc.
|
|
@jni No offense taken. This PR can be closed since I rolled my own implementation and I'm not using this function anymore. Here are just my two cents on this:
But you can count me out from now. |
|
I think @jni's point is that those points on the border that seem like maxima may very well not be, had we been able to sample "further afield". On the other hand, if you define a maximum as a pixel with no greater neighbor, then these count. So, yes, this should remain an option, and it is not clear what to make the default—either option seems fine, as long as it can easily be disabled. The current status is just weird, since it defaults to "True", which doesn't seem right—should be 1 or 0. The docstring states: So, what happens to |
|
@stefanv if |
Fixes #2407