Ensure watershed auto-markers respect mask

[jni]

Description

Fixes #3808

Additionally, I took the opportunity to allow watershed without passing in markers. Then the local minima in the image are used.

Checklist

• 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
• Clean style in the spirit of PEP8

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.
• Consider backporting the PR with @meeseeksdev backport to v0.14.x

[pep8speaks]

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

• In the file skimage/morphology/_util.py:

Line 82:50: W504 line break after binary operator

• In the file skimage/morphology/tests/test_watershed.py:

Line 55:80: E501 line too long (98 > 79 characters)
Line 56:80: E501 line too long (98 > 79 characters)
Line 57:80: E501 line too long (98 > 79 characters)
Line 58:80: E501 line too long (98 > 79 characters)
Line 59:80: E501 line too long (98 > 79 characters)
Line 60:43: E241 multiple spaces after ','
Line 60:48: E241 multiple spaces after ','
Line 60:53: E241 multiple spaces after ','
Line 60:58: E241 multiple spaces after ','
Line 60:63: E241 multiple spaces after ','
Line 60:68: E241 multiple spaces after ','
Line 60:80: E501 line too long (98 > 79 characters)
Line 61:43: E241 multiple spaces after ','
Line 61:48: E241 multiple spaces after ','
Line 61:53: E241 multiple spaces after ','
Line 61:58: E241 multiple spaces after ','
Line 61:63: E241 multiple spaces after ','
Line 61:68: E241 multiple spaces after ','
Line 61:80: E501 line too long (98 > 79 characters)
Line 62:48: E241 multiple spaces after ','
Line 62:53: E241 multiple spaces after ','
Line 62:58: E241 multiple spaces after ','
Line 62:63: E241 multiple spaces after ','
Line 62:80: E501 line too long (98 > 79 characters)
Line 63:53: E241 multiple spaces after ','
Line 63:58: E241 multiple spaces after ','
Line 63:80: E501 line too long (98 > 79 characters)
Line 64:53: E241 multiple spaces after ','
Line 64:58: E241 multiple spaces after ','
Line 64:80: E501 line too long (98 > 79 characters)
Line 65:80: E501 line too long (98 > 79 characters)
Line 66:53: E241 multiple spaces after ','
Line 66:58: E241 multiple spaces after ','
Line 66:80: E501 line too long (98 > 79 characters)
Line 67:53: E241 multiple spaces after ','
Line 67:58: E241 multiple spaces after ','
Line 67:80: E501 line too long (98 > 79 characters)
Line 68:48: E241 multiple spaces after ','
Line 68:53: E241 multiple spaces after ','
Line 68:58: E241 multiple spaces after ','
Line 68:63: E241 multiple spaces after ','
Line 68:80: E501 line too long (98 > 79 characters)
Line 69:43: E241 multiple spaces after ','
Line 69:48: E241 multiple spaces after ','
Line 69:53: E241 multiple spaces after ','
Line 69:58: E241 multiple spaces after ','
Line 69:63: E241 multiple spaces after ','
Line 69:68: E241 multiple spaces after ','
Line 69:80: E501 line too long (98 > 79 characters)
Line 70:43: E241 multiple spaces after ','
Line 70:48: E241 multiple spaces after ','
Line 70:53: E241 multiple spaces after ','
Line 70:58: E241 multiple spaces after ','
Line 70:63: E241 multiple spaces after ','
Line 70:68: E241 multiple spaces after ','
Line 70:80: E501 line too long (98 > 79 characters)
Line 71:80: E501 line too long (98 > 79 characters)
Line 72:80: E501 line too long (98 > 79 characters)
Line 73:80: E501 line too long (98 > 79 characters)
Line 74:80: E501 line too long (98 > 79 characters)
Line 75:80: E501 line too long (99 > 79 characters)

Comment last updated at 2019-03-26 12:26:38 UTC

[lagru]

Only some small comments / questions. Using local minima as seed points in case of None is nice new feature!

[lagru]

What's the reasoning for making this a lazy import?

[jni]

extrema imports from watershed (the offsets to raveled neighbors), so not using this resulted in a circular import. I guess the correct way is to move offsets to its own utility file.

[lagru]

Not sure if I missed something but wouldn't it be more readable to remove the not and switch the bodies of this elif- and the following else-statements? Maybe even test for "int-ness" and add a third branch that raises a nice TypeError if markers has an unsupported type?

[jni]

Possibly this is true, I'm not sure about the original rationale, this is not new code. I'll change it.

[lagru]

Nitpick: I think "no markers" may be confused with giving 0 or passing in an array that is False everywhere.

Suggested change

	no markers are given, the local minima of the image are used as
	no ``None``, the local minima of the image are used as

Maybe remove "Zero means not a marker." and just display a warning if the input leads to no seeds being defined. Is there a use case where this doesn't hint at a usage error?

[emmanuelle]

If None (no markers provided), the local minimal of the image...

[emmanuelle]

Why do you call this markers? For me markers and seeds are synonyms, corresponding to marked pixels from which the watershed flooding is done. How about "labeled pixels"? Sorry to be nitpicking about a comment in a test ;-).

[emmanuelle]

OK, I think I got it, we're talking about the same thing for markers, you just want to check that no marker was chosen outside the mask, so that all pixels outside the mask in the segmentation are set to zero. OK!

[emmanuelle]

The code looks fine to me. Just a question, why did you use local_minimal and not peak_local_max? With peak_local_max it would be possible to add a minimal distance between local minima, but then it adds another parameter... Well, I guess the watershed only provides good results with well-chosen markers anyway.

[jni]

@emmanuelle we've had a lot of issues with peak_local_max being buggy or having strange behaviour, many of which are unresolved, see:

https://github.com/scikit-image/scikit-image/issues?utf8=✓&q=is%3Aissue+is%3Aopen+peak_local_max

So I much prefer using local_minima, which actually matches the original watershed paper anyway. I'm less interested in making the watershed function do all possible seeding strategies than in providing seedless watershed as originally proposed.

[jni]

@lagru @emmanuelle I believe I have addressed your concerns. =)

[jni]

Hmm, failure in Py3.7 that seems unrelated to this PR:

Warning, treated as error:
/home/travis/build/scikit-image/scikit-image/doc/source/auto_examples/color_exposure/plot_rgb_to_hsv.rst:26:Explicit markup ends without a blank line; unexpected unindent.

I'll see if I can fix it.

[emmanuelle]

Merging, thanks!