Hi @haesleinhuepf thanks for starting this discussion! I'll let other devs chime in, but my gut feeling is that rather than adding one function to the API we could just add one keyword argument histogram (which would default to None) to the existing function. I see the interest of precomputing the histogram also for comparing different thresholding functions which require the histogram.

@sciunto you've worked extensively on thresholding functions what do you think of these additions?

I think the same as @emmanuelle: let's add histogram=None to the functions that benefit from a pre-computed histogram. We could potentially then also make image=None, and make sure one of the two is provided, with preference given to the histogram.

Hey @emmanuelle and @jni ,

at the beginning I thought so, too. But then I saw that we need the histogram and the bin_centers array. Otherwise, we can only return the bin_index and not the corresponding grey value. I didn't know how force the user to provide both together or both none. Assert statements came to my mind. Any better idea how to achieve this?

Thanks for the feedback! 😃

A great point that I had overlooked! 😅

My preferred option given this would be to provide a tuple of counts, bin_centers, same as returned by exposure.histogram. Perhaps we also support a single array and then use the bin index (bin_centers = np.arange(len(counts))), which is convenient for uint16 and uint8 images.

Perhaps we also support a single array and then use the bin index (bin_centers = np.arange(len(counts))), which is convenient for uint16 and uint8 images.

Just to make sure I understand correctly: That would mean the threshold_otsu method returns a conceptually different thing (histogram index versus grey value) depending on input parameters. If that's ok, I'm happy to implement it.

I'm +1 with you @emmanuelle and this can be applied to several of our functions as well.

a conceptually different thing

It's not, the point is that for integer-valued images they are one and the same thing! So it is a shortcut for users of those images. If someone decides to misuse the shortcut, that's on them! 😜

At any rate, you can implement the tuple input version first and later we can decide if we want to provide this shortcut. 😊

Hello @haesleinhuepf! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

• In the file skimage/filters/thresholding.py:

Line 292:80: E501 line too long (81 > 79 characters)

Comment last updated at 2020-10-05 15:03:51 UTC

Alright @emmanuelle and @jni ,

I just implemented it like you suggested. The function header is now

def threshold_otsu(image=None, nbins=256, hist=None, bin_centers=None)

If prefers the hist over the image as @jni suggested and returns the bin index in case no bin_centers are given. The only thing I didn't manage is the tuple-thingy @jni suggested. I assume one needs to add some more brakets to the function defintion, but I'm not sure how this works with bin_centers being optional. You know, I just recently arrived in snake county. ;-)

Thanks again for your feedback. I appreciate it :-)

Let me know what you think!

Cheers,
Robert

@haesleinhuepf I think you were expecting more magic than Python provides here. 😂 I merely meant that a single argument, histogram=, takes in as input a tuple of arrays, which you unpack inside the function:

def threshold_otsu(image=None, nbins=256, *, histogram=None):
    if histogram is not None:
        counts, bin_centers = histogram
    else:
        counts, bin_centers = exposure.histogram(image, nbins)

or

def threshold_otsu(image=None, nbins=256, *, histogram=None):
    if histogram is not None:
        if isinstance(histogram, (tuple, list)):
            counts, bin_centers = histogram
        else:
            counts = histogram
            bin_centers = np.arange(counts.size)
    else:
        counts, bin_centers = exposure.histogram(image, nbins)