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

Precompute histogram to speed up try_all_threshold #5305

Merged
merged 4 commits into from
Jul 31, 2021

Conversation

sciunto
Copy link
Member

@sciunto sciunto commented Mar 31, 2021

Description

We recently merged a PR to use pre-computed histograms in threshold functions. This feature can be used internally in try_all_threshold. I noticed a 2x improvement on the hubble image.

Checklist

For reviewers

  • 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.

@jni
Copy link
Member

jni commented Mar 31, 2021

@sciunto you got us to go 2x faster to a test failure! 😂

____________________ TestSimpleImage.test_try_all_threshold ____________________

self = <skimage.filters.tests.test_thresholding.TestSimpleImage object at 0x7fd0a6a16cd0>

    def test_try_all_threshold(self):
        fig, ax = try_all_threshold(self.image)
        all_texts = [axis.texts for axis in ax if axis.texts != []]
        text_content = [text.get_text() for x in all_texts for text in x]
>       assert 'RuntimeError' in text_content
E       AssertionError: assert 'RuntimeError' in ['TypeError', 'TypeError', 'TypeError', 'TypeError', 'TypeError', 'TypeError', ...]

scikit-image/skimage/filters/tests/test_thresholding.py:48: AssertionError

Not all functions take a histogram, so probably this needs to check, either using the inspect module or in a try/except.

@sciunto
Copy link
Member Author

sciunto commented Mar 31, 2021

Why this didn't happen during my test? 🤔

Copy link
Contributor

@grlee77 grlee77 left a comment

Choose a reason for hiding this comment

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

LGTM. The build failure looks like an unrelated time-out on Azure.

Copy link
Member

@mkcor mkcor left a comment

Choose a reason for hiding this comment

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

Nice! It looks like this contribution almost fell in the cracks...

skimage/filters/thresholding.py Outdated Show resolved Hide resolved
@mkcor mkcor merged commit b7d0209 into scikit-image:main Jul 31, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants