draw_random_shapes: add background argument and avoid invisible objects #3216

cmohl2013 · 2018-06-21T15:09:08Z

Description

The new optional background argument defines a custom background color. Before, background was hard coded to 255 for each channel.

The PR also includes a fix for randomly picking colors of foreground objects: Foreground objects are not allowed to have background color any more (i.e. making them invisible).

Checklist

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

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

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

References

Closes #3083

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

stefanv

Thanks for your contribution, Christoph. I've left some initial feedback.

stefanv · 2018-06-21T16:56:15Z

skimage/draw/_random_shapes.py

+    else:
+        intensity_range_tpl = intensity_range
+
+    if np.diff(np.array(intensity_range_tpl)).sum() == 0:


Is this check overkill? Even if not, I think the error message should at least be improved. But what do we lose if we remove it entirely?

If intensity range spans only a single color, and this color equals the color to be excluded, the recursive call in line 230 will end up in an infinite loop. Therefore this check is important. The error message could be more specific, indeed.

stefanv · 2018-06-21T16:58:53Z

skimage/draw/_random_shapes.py

+              for r in intensity_range_tpl]
+    colors = np.transpose(colors)
+
+    to_replace = np.array([(exclude == color).all() for color in colors])


First convert to a numpy array and then perform array operations?

Yes, would be more efficient.

something like this: to_replace = np.equal(color, exclude).all(axis=1)

stefanv · 2018-06-21T17:00:03Z

skimage/draw/_random_shapes.py

+    if background is None:
+        background = [255] * num_channels
+    elif isinstance(background, int):
+        background = [background]


[background] * num_channels?

Also, what about the case where the user wants to specify RGB background.

[background] * num_channels: good idea. My initial idea was to not allow single values for background in multichannel images. But this is more convenient.

I'm not shure if I understand you correctly regarding RGB background. The user can specify background for images of three channels with a tuple of length 3. Or do you think of matplotlib-like color strings as e.g.background='r' for red background?

stefanv · 2018-06-22T19:17:21Z

No, I was thinking of the 3-tuple case. I missed seeing that that is already supported.

soupault

@cmohl2013 very good starting point, thanks! Let's polish the PR a bit. :)

soupault · 2018-06-22T20:12:11Z

skimage/draw/_random_shapes.py

+        background = [background]
+    if len(background) != num_channels:
+        raise ValueError('Nr of background values must match nr of channels')
+    for intensity in background:


It seems that operations on background take too many lines. Let's just cast it to ndarray from the beginning, and use the vectorized ops.

soupault · 2018-06-22T20:14:00Z

skimage/draw/_random_shapes.py

+        ranges are equal across the channels, and
+        ((min_0, max_0), ... (min_N, max_N)) if they differ. As the function
+        supports generation of uint8 arrays only, the maximum range is
+        (0, 255). If None, set to (0, 255) for each channel reserving color of


reserving color of intensity = 255 for background. <- this should probably be moreved. The same info is provided 2 lines below.

soupault · 2018-06-22T20:16:32Z

skimage/draw/_random_shapes.py

-        intensity_range = intensity_range * num_channels
+        intensity_range_tpl = intensity_range * num_channels
+    else:
+        intensity_range_tpl = intensity_range


Is there really a need in changing this block?

I wanted to keep the original intensity_range value for using in the recursive call in line 230. But I see that this approach is confusing, makes the code less readable. Indeed this can be done outside the block in a more simple way.

soupault · 2018-06-22T20:19:59Z

skimage/draw/_random_shapes.py

+    else:
+        intensity_range_tpl = intensity_range
+
+    if np.diff(np.array(intensity_range_tpl)).sum() == 0:


.sum() is not a 100% valid check here. Will fail on, for example, ((0, 1), (1, 0), (0, 0)).

soupault · 2018-06-22T20:21:35Z

skimage/draw/_random_shapes.py

+
+    if np.diff(np.array(intensity_range_tpl)).sum() == 0:
+        # if intensity range only spans a single color
+        colors = np.array([np.array(intensity_range_tpl)[:, 0]])


To many np.array constructors here, above, and below. Please, cast intensity_range_tpl to ndarray as early as possible, and use the vectorized ops.

Sorry to interject, but asarray might be appropriate to avoid a copy early on.

Also, are you looking for np.newaxis. you would slice with [:, 0, np.newaxis]

@hmaarrfk thanks for the hint!

soupault · 2018-06-22T20:23:05Z

skimage/draw/_random_shapes.py

+    elif isinstance(background, int):
+        background = [background]
+    if len(background) != num_channels:
+        raise ValueError('Nr of background values must match nr of channels')


Nr/nr abbreviation should be expanded.

soupault · 2018-06-22T20:25:02Z

skimage/draw/tests/test_random_shapes.py

-def test_generates_white_image_when_intensity_range_255():
-    image, labels = random_shapes((128, 128), max_shapes=3,
-                                  intensity_range=((255, 255),),
+def test_throws_when_intensity_range_255():


test_throws_when_intensity_range_255 -> test_throws_when_intensity_range_equals_background or something. Also, please specify the background value explicitly in the function call below.

soupault · 2018-06-22T20:26:41Z

skimage/draw/tests/test_random_shapes.py

+                      random_seed=42)
+
+
+def test_pick_random_colors_within_range():


This could be rewritten using parametrize decorator.

soupault · 2018-06-22T20:30:47Z

skimage/draw/tests/test_random_shapes.py

+        assert tuple(color) in [(20, 25), (20, 26), (21, 26)]
+
+
+def test_throws_when_intensity_range_equals_excluded_intensities():


Erm, this essentially repeats test_throws_when_intensity_range_255...

soupault · 2018-06-22T20:32:23Z

skimage/draw/tests/test_random_shapes.py

+    assert set(image[:, :, 1].flatten()) == {30, 31}
+
+
+def test_throws_when_backgound_out_of_range():


This function and the one below could be also refactored using parametrize.

cmohl2013 · 2018-06-23T22:31:32Z

@stefanv @soupault Thank you for your descent review! I think all the improvements you suggested are clear to me. I'll try to update the PR in the next couple of days.

pep8speaks · 2018-07-10T16:01:48Z

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

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2020-11-01 00:48:12 UTC

cmohl2013 · 2018-07-10T16:07:32Z

I updated the PR according the comments. Should the example in the gallery also be updated with a background argument example?

soupault · 2018-08-11T17:53:13Z

skimage/draw/_random_shapes.py

+
+    if (np.diff(intensity_range) == 0).all():
+        # if intensity range only spans a single color
+        colors = np.broadcast_to(intensity_range[:, 0],


How about switching to .repeat or .tile?

soupault · 2018-08-11T17:56:23Z

skimage/draw/_random_shapes.py

+    background : {tuple of ints, int}, optional
+        Pixel intensities for background. Values between 0 and 255 are allowed.
+        For multichannel, a tuple of length num_channels is required. If None,
+        set to 255 for each channel.


Does it really make sense to support None?

You're right, better we do not support None. Solves also some of your comments below.

soupault · 2018-08-11T18:01:13Z

skimage/draw/_random_shapes.py

+    elif len(background) != num_channels:
+        msg = 'Number of background values must match number of channels.'
+        raise ValueError(msg)
+    if (background <= 0).any() or (background > 255).any():


0 is a valid value.

soupault · 2018-08-11T18:05:25Z

skimage/draw/_random_shapes.py

@@ -171,7 +171,8 @@ def _generate_triangle_mask(point, image, shape, random):
 SHAPE_CHOICES = list(SHAPE_GENERATORS.values())


-def _generate_random_colors(num_colors, num_channels, intensity_range, random):
+def _generate_random_colors(num_colors, num_channels, intensity_range, random,
+                            exclude=None):


The effect of None should be described in the docstring.

soupault · 2018-08-11T18:11:08Z

skimage/draw/_random_shapes.py

+        # if intensity range only spans a single color
+        colors = np.broadcast_to(intensity_range[:, 0],
+                                 (num_colors, num_channels))
+        msg = ('Shape colors can not be selected.' +


Move this under the if loop.
You can remove + inside the parentheses. Also, add a whitespace after the period (the strings are concatenated).

soupault · 2018-08-11T18:16:06Z

skimage/draw/_random_shapes.py

    random = np.random.RandomState(random_seed)
    user_shape = shape
    image_shape = (image_shape[0], image_shape[1], num_channels)
-    image = np.ones(image_shape, dtype=np.uint8) * 255
+    image = np.ones(image_shape, dtype=np.uint8) * background


Multiplying by None is not what you probably want.

soupault · 2018-08-11T18:19:51Z

skimage/draw/_random_shapes.py

+        if exclude is not None and (colors[0, :] == exclude).all():
+            raise ValueError(msg)
+        return colors
+


soupault · 2018-08-11T18:23:59Z

skimage/draw/_random_shapes.py

+
+    to_replace = (colors == exclude).all(axis=1)
+    if to_replace.any():
+        colors[to_replace, :] = _generate_random_colors(sum(to_replace),


This can actually cause stack overflow (highly unlikely, but still). I think the sampling should be implemented in a different way.

soupault · 2018-08-11T18:25:27Z

@cmohl2013 Thank you for the changes! The current implementation is much better, although, I still see some issues (see the comments above).

cmohl2013 · 2018-08-15T13:42:31Z

@soupault Thank you again for the review. I'll try to change implementation according your comments.

…to _generate_random_colors

…s <1.13

cmohl2013 · 2018-10-26T16:58:35Z

@soupault
I implemented changes according to your requests.

soupault

@cmohl2013 I think, I've found a bug in the current implementation. Once fixed, this PR should be good to go.

skimage/draw/_random_shapes.py

soupault · 2018-11-07T15:52:00Z

skimage/draw/_random_shapes.py

+        else:
+            return colors
+    color_candidates = [np.arange(r[0], r[1] + 1) for r in intensity_range]
+    color_candidates = [np.setdiff1d(c, e) for c, e


I think, this might, occasionally, remove some valid colors:

cc = np.array([[1, 2, 3], [4, 5, 6]]) exclude = np.array([1, 4]) cc = [np.setdiff1d(c, e) for c, e in zip(color_candidates, exclude)] cc -> [array([2, 3]), array([5, 6])]

In this case, [1, 5], [1, 6], [2, 4], [3, 4] are valid foreground values, but no longer considered.

Definitely, would need a test for this :).

wow, a bug indeed, not so easy to find I guess. Yes I'll add a test and see how to fix this. Thanks!

soupault · 2018-11-07T15:52:54Z

skimage/draw/_random_shapes.py

+        # if intensity range only spans a single color
+        colors = np.broadcast_to(intensity_range[:, 0],
+                                 (num_colors, num_channels))
+        if exclude is not None and (colors[0, :] == exclude).all():


Personally, I feel that putting this check before the broadcasting would be more coherent.

soupault · 2018-11-07T15:55:29Z

skimage/draw/_random_shapes.py

+    exclude : {tuple of ints, int}, optional
+        A color within the intensity_range that is excluded from
+        random sampling. For multichannel, a tuple of length num_channels is
+        required. If `None`, no color is excluded.


You should say that you're broadcasting a single value in the multichannel case (effectively).

Co-Authored-By: cmohl2013 <cmohl@yahoo.com>

stefanv reviewed Jun 21, 2018

View reviewed changes

soupault added the ⏩ type: Enhancement Improve existing features label Jun 21, 2018

soupault added this to the 0.15 milestone Jun 21, 2018

soupault self-assigned this Jun 21, 2018

soupault reviewed Jun 22, 2018

View reviewed changes

cmohl2013 force-pushed the test branch from aa0ef01 to 7217724 Compare July 10, 2018 16:01

soupault requested changes Aug 11, 2018

View reviewed changes

cmohl2013 force-pushed the test branch from 7217724 to a44ac71 Compare October 19, 2018 13:46

Christoph Möhl added 6 commits October 26, 2018 14:02

added background argument to draw_random_shapes and exclude argument …

9278ab7

…to _generate_random_colors

added tests for value errors and added docstr

8868a79

replaced np.isin in _generate_random_shapes to work for numpy version…

a6f1272

…s <1.13

changes according comments of soupault and hmaarrfk

e05cb8a

refactored tests

82d796a

implemented changes according @soupault s comments

eda19c8

cmohl2013 force-pushed the test branch from a44ac71 to eda19c8 Compare October 26, 2018 13:28

Christoph Möhl added 2 commits October 26, 2018 16:26

added gallery example

a7b8446

pep8

1276d52

soupault requested changes Nov 7, 2018

View reviewed changes

format documentation

a1faf27

Co-Authored-By: cmohl2013 <cmohl@yahoo.com>

soupault modified the milestones: 0.15, 0.16 Apr 20, 2019

sciunto removed this from the 0.16 milestone Mar 8, 2020

sciunto added the 💪 Work in progress label Mar 8, 2020

Merge branch 'master' into test

0846b2c

Base automatically changed from master to main February 18, 2021 18:23

mkcor mentioned this pull request Feb 22, 2021

2021's calendar of community management #5169

Closed

scikit-image deleted a comment from codecov-io Oct 27, 2023

		assert tuple(color) in [(20, 25), (20, 26), (21, 26)]


		def test_throws_when_intensity_range_equals_excluded_intensities():

		assert set(image[:, :, 1].flatten()) == {30, 31}


		def test_throws_when_backgound_out_of_range():

draw_random_shapes: add background argument and avoid invisible objects #3216

Are you sure you want to change the base?

draw_random_shapes: add background argument and avoid invisible objects #3216

Conversation

cmohl2013 commented Jun 21, 2018 • edited

Description

Checklist

References

For reviewers

stefanv left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stefanv commented Jun 22, 2018 via email

soupault left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cmohl2013 commented Jun 23, 2018

pep8speaks commented Jul 10, 2018 • edited

Comment last updated at 2020-11-01 00:48:12 UTC

cmohl2013 commented Jul 10, 2018

soupault Aug 11, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

soupault commented Aug 11, 2018

cmohl2013 commented Aug 15, 2018

cmohl2013 commented Oct 26, 2018

soupault left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cmohl2013 commented Jun 21, 2018 •

edited

pep8speaks commented Jul 10, 2018 •

edited

soupault Aug 11, 2018 •

edited