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

New function: skimage.filters.try_all_threshold_dict (counterpart to try_all_threshold) #3207

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

GenevieveBuckley
Copy link
Contributor

Description

A new function skimage.filters.try_all_threshold_dict is implemented.

An ordered dictionary is returned from skimage.filters.try_all_threshold_dict (keys = threshold method names, values = numeric value or None), whereas its counterpart skimage.filters.try_all_threshold returns a matplotlib visualisation.

>>> from skimage import data
>>> from skimage.filters import try_all_threshold_dict
>>> img = data.page()
>>> threshold_results = try_all_threshold_dict(img)
>>> for method, result in threshold_results.items():
...    print("{} threshold: {}".format(method, result))

Isodata threshold: 157
Li threshold: 148.1795556988551
Mean threshold: 171.54482984293193
Minimum threshold: 191
Otsu threshold: 157
Triangle threshold: 206
Yen threshold: 121

This is implemented as a separate function to skimage.filters.try_all_threshold for the reasons @jni outlines here:

As we are aiming to remove our matplotlib dependency in the medium term, probably the right approach is to have separate functions for visualising, rather than a keyword argument. This will make it easier to contain all mpl functionality in a single sub-package.

Checklist

[It's fine to submit PRs which are a work in progress! But before they are merged, all PRs should provide:]

[For detailed information on these and other aspects see scikit-image contribution guidelines]

References

First suggested by @stefanv in 2178, and requested by @emmanuelle in 3149.

For reviewers

(Don't remove the checklist below.)

  • Check that the PR title is short, concise, and will make sense 1 year
    later.
  • Check that new functions are imported in corresponding __init__.py.
  • Check that new features, API changes, and deprecations are mentioned in
    doc/release/release_dev.rst.
  • Consider backporting the PR with @meeseeksdev backport to v0.14.x

@pep8speaks
Copy link

Hello @GenevieveBuckley! Thanks for submitting the PR.

Line 65:1: E402 module level import not at top of file
Line 78:1: E402 module level import not at top of file
Line 79:1: E402 module level import not at top of file

Line 46:1: E402 module level import not at top of file
Line 47:1: E402 module level import not at top of file

@@ -134,6 +135,59 @@ def wrapper(im):
methods=methods, verbose=verbose)


def try_all_threshold_dict(image):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't find this name particularly elegant. Ideally, I would prefer try_all_threshold -> dict and try_all_threshold_plotted -> mpl. But this would require a deprecation cycle...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed; have try_all_threshold always return results, and then add a visualize flag?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@stefanv that makes it harder/impossible to return the figure and axes as done currently. At any rate, if long term we want to remove the mpl dependency for analysis, we could have try_all_threshold return a dict, and then have a separate function in e.g. the viewer (or whatever we call the skimage-mpl package) to visualise that dictionary.

Either way, yes, a deprecation is going to be needed. =\

>>> dict = try_all_threshold_dict(text())
"""
# Global algorithms.
methods = OrderedDict({'Isodata': threshold_isodata,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would make sense to me to do not duplicate this list in the code in order to make the addition of algorithms more straightfoward.

@sciunto
Copy link
Member

sciunto commented Jun 18, 2018

I made 2 comments, but it's open for discussion.

@stefanv
Copy link
Member

stefanv commented Jun 20, 2018 via email

@alexdesiqueira
Copy link
Member

alexdesiqueira commented May 29, 2019

Hey all,
I'm arriving kinda late to the party, but here's my suggestion.
Why not joining the three approaches: try_all_threshold() returning a dict (as @GenevieveBuckley and @stefanv proposed), and put an appropriate function to plot all of it in our viewer, such as viewer.view_all_thresholds() (@jni's suggestion)? This second function could have more structure, receive nrows, ncols, and so on.
We'd have two functions easier to handle, and our documentation could approach everything in a nice way.

Base automatically changed from master to main February 18, 2021 18:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants