Fix morphology.local_maxima for input with any dimension < 3 by lagru · Pull Request #3447 · scikit-image/scikit-image

lagru · 2018-10-06T18:25:10Z

Description

local_maxima (and local_minima) failed with a ValueError for images with at least 1 dimension with fewer than 3 elements. This was due to the implementation which applies a structuring element with exactly 3 elements in each dimension to the image in question. This behavior was only visible if allow_borders was false, otherwise the image was padded internally.
This fix makes these functions work for the mentioned edge cases by testing the image shape at the appropriate time.
The output of local_maxima([], indices=True) didn't match the output for non-empty arrays and the description of the docstring. This is fixed as well.
Replaced assert_equal with assert_array_equal which is more restrictive and (I think) more inline with what we actually want to test.

Checklist

Clean style in the spirit of PEP8
Docstrings for all functions
~~Gallery example in ./doc/examples (new features only)~~
~~Benchmark in ./benchmarks, if your changes aren't covered by an existing benchmark~~
Unit tests

References

Closes #3261

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

local_maxima (and local_minima) failed with an ValueError for images with at least 1 dimension with fewer than 3 elements. This was due to the implementation which applies a structuring element with exactly 3 elements in each dimension to the image in question. This behavior was only visible if `allow_borders` was false, otherwise the image was padded internally. This fix makes these functions work for the mentioned edge cases by testing the image shape at the appropriate time.

codecov-io · 2018-10-06T18:58:43Z

Codecov Report

Merging #3447 into master will increase coverage by 0.34%.
The diff coverage is 97.77%.

@@            Coverage Diff             @@
##           master    #3447      +/-   ##
==========================================
+ Coverage   86.86%   87.21%   +0.34%     
==========================================
  Files         340      342       +2     
  Lines       27485    27769     +284     
==========================================
+ Hits        23875    24218     +343     
+ Misses       3610     3551      -59

Impacted Files	Coverage Δ
skimage/morphology/tests/test_extrema.py	`99.48% <100%> (+0.07%)`	⬆️
skimage/morphology/extrema.py	`95.14% <93.75%> (+0.24%)`	⬆️
skimage/io/_plugins/gtk_plugin.py	`25% <0%> (-5.56%)`	⬇️
skimage/future/manual_segmentation.py	`10.78% <0%> (-2.09%)`	⬇️
skimage/io/_plugins/util.py	`59.39% <0%> (-1.82%)`	⬇️
skimage/draw/draw.py	`98.11% <0%> (-0.84%)`	⬇️
skimage/measure/tests/test_regionprops.py	`99.64% <0%> (-0.36%)`	⬇️
skimage/segmentation/active_contour_model.py	`98.96% <0%> (-0.02%)`	⬇️
skimage/color/colorconv.py	`99.08% <0%> (-0.01%)`	⬇️
skimage/restoration/unwrap.py	`100% <0%> (ø)`	⬆️
... and 33 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update af8a78e...bd72727. Read the comment docs.

skimage/morphology/tests/test_extrema.py

emmanuelle · 2018-10-08T21:01:32Z

Thanks @lagru . Should a warning be raised when one of the dimensions is <3 (in order to help the user understand why no maxima are detected)?

lagru · 2018-10-08T21:35:54Z

Should a warning be raised when one of the dimensions is <3 (in order to help the user understand why no maxima are detected)?

Why not. Good idea! I'll add it.

The old output for empty arrays if `indices` was true didn't correspond to the output for non-empty arrays and the description in the docstring. This ensures that the output will always be a tuple of empty arrays. If `indices` is false, the returned empty array now machtes the shape of `image` as well.

which is the case for dimensions with fewer than 3 samples. Because `local_minima` wraps `local_maxima` the warning can only set the correct stacklevel for one of these functions. The latter was chosen because it stands to reason that it is / will be used more frequently. If may be possible to suppress and re-raise the warning with the context manager from the warnings module but that would introduce non-thread- safe code and introduce perhaps unnecessary complexity.

lagru · 2018-10-09T09:38:36Z

skimage/morphology/extrema.py

+        # -> no maxima can exist & structuring element can't be applied
+        warn(
+            "no maxima can exist for an image with any dimension smaller 3",
+            stacklevel=3


See #2097 (comment).

sciunto · 2018-11-11T13:25:13Z

@lagru What's the status of this PR?

thank you.

lagru · 2018-11-11T15:49:44Z

@sciunto As far as I remember I this is ready to merge (test failure is unrelated). However because this PR involved adding a warning I decided to wait until a decision is made how scikit-image wants to raise warnings (see #2097, #3438 and #3467).

By the way, because local_minima wraps local_maxima the warning can only have the correct stacklevel for one of these functions (currently the latter).

sciunto · 2018-11-11T19:55:53Z

I restarted the test, it's passing now.

skimage/morphology/extrema.py

jni

I've left two minor comments. Happy for this to be merged as-is, or they can be addressed and then merged.

jni · 2018-11-11T23:46:41Z

Thanks @lagru!

to reduce visual clutter.

stefanv · 2018-11-13T20:32:12Z

Thanks for catching these; I love the guiding warnings and error messages.

…image#3447) * Fix local_maxima for input with dimensions < 3 local_maxima (and local_minima) failed with an ValueError for images with at least 1 dimension with fewer than 3 elements. This was due to the implementation which applies a structuring element with exactly 3 elements in each dimension to the image in question. This behavior was only visible if `allow_borders` was false, otherwise the image was padded internally. This fix makes these functions work for the mentioned edge cases by testing the image shape at the appropriate time. * Fix output of local_maxima for empty arrays The old output for empty arrays if `indices` was true didn't correspond to the output for non-empty arrays and the description in the docstring. This ensures that the output will always be a tuple of empty arrays. If `indices` is false, the returned empty array now machtes the shape of `image` as well. * Show warning if no maxima are possible which is the case for dimensions with fewer than 3 samples. Because `local_minima` wraps `local_maxima` the warning can only set the correct stacklevel for one of these functions. The latter was chosen because it stands to reason that it is / will be used more frequently. If may be possible to suppress and re-raise the warning with the context manager from the warnings module but that would introduce non-thread- safe code and introduce perhaps unnecessary complexity. * Point user to allow_borders in warning message

stefanv · 2018-11-13T20:45:05Z

Manual backport at #3545

lagru · 2018-11-14T08:01:39Z

You're welcome! 🙂

…-image#3447) * Fix local_maxima for input with dimensions < 3 local_maxima (and local_minima) failed with an ValueError for images with at least 1 dimension with fewer than 3 elements. This was due to the implementation which applies a structuring element with exactly 3 elements in each dimension to the image in question. This behavior was only visible if `allow_borders` was false, otherwise the image was padded internally. This fix makes these functions work for the mentioned edge cases by testing the image shape at the appropriate time. * Fix output of local_maxima for empty arrays The old output for empty arrays if `indices` was true didn't correspond to the output for non-empty arrays and the description in the docstring. This ensures that the output will always be a tuple of empty arrays. If `indices` is false, the returned empty array now machtes the shape of `image` as well. * Show warning if no maxima are possible which is the case for dimensions with fewer than 3 samples. Because `local_minima` wraps `local_maxima` the warning can only set the correct stacklevel for one of these functions. The latter was chosen because it stands to reason that it is / will be used more frequently. If may be possible to suppress and re-raise the warning with the context manager from the warnings module but that would introduce non-thread- safe code and introduce perhaps unnecessary complexity. * Point user to allow_borders in warning message

Backport #3022 & #3447: Rewrite of local_maxima with flood-fill & Cython + bugfix

emmanuelle reviewed Oct 8, 2018

View reviewed changes

skimage/morphology/tests/test_extrema.py Outdated Show resolved Hide resolved

lagru added 2 commits October 9, 2018 11:26

lagru force-pushed the fix-extrema branch from f594095 to 9e8273c Compare October 9, 2018 09:35

lagru commented Oct 9, 2018

View reviewed changes

sciunto added this to the 0.15 milestone Nov 11, 2018

jni reviewed Nov 11, 2018

View reviewed changes

skimage/morphology/extrema.py Outdated Show resolved Hide resolved

jni approved these changes Nov 11, 2018

View reviewed changes

lagru added 2 commits November 12, 2018 11:28

Revert assert_array_equal to assert_equal

61caee2

to reduce visual clutter.

Point user to allow_borders in warning message

bd72727

stefanv approved these changes Nov 13, 2018

View reviewed changes

stefanv merged commit 29a22e1 into scikit-image:master Nov 13, 2018

lumberbot-app bot added the Still Needs Manual Backport MrMeeseeks-managed label label Nov 13, 2018

scikit-image deleted a comment from lumberbot-app bot Nov 13, 2018

lagru deleted the fix-extrema branch November 14, 2018 08:02

lagru mentioned this pull request Nov 14, 2018

Backport #3022 & #3447: Rewrite of local_maxima with flood-fill & Cython + bugfix #3546

Merged

9 tasks

jni added a commit that referenced this pull request Nov 14, 2018

Merge pull request #3546 from lagru/backport_v0.14.x/3022_3447

d2a05a0

Backport #3022 & #3447: Rewrite of local_maxima with flood-fill & Cython + bugfix

Uh oh!

Conversation

lagru commented Oct 6, 2018

Description

Checklist

References

For reviewers

Uh oh!

codecov-io commented Oct 6, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

emmanuelle commented Oct 8, 2018

Uh oh!

lagru commented Oct 8, 2018

Uh oh!

lagru Oct 9, 2018

Choose a reason for hiding this comment

Uh oh!

sciunto commented Nov 11, 2018

Uh oh!

lagru commented Nov 11, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sciunto commented Nov 11, 2018

Uh oh!

Uh oh!

jni left a comment

Choose a reason for hiding this comment

Uh oh!

jni commented Nov 11, 2018

Uh oh!

stefanv commented Nov 13, 2018

Uh oh!

stefanv commented Nov 13, 2018

Uh oh!

lagru commented Nov 14, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

codecov-io commented Oct 6, 2018 •

edited

Loading

lagru commented Nov 11, 2018 •

edited

Loading