Add `colors` support for `tf.image.draw_bounding_boxes` #24282

yongtang · 2018-12-11T06:10:02Z

This fix tries to address the issue raised in #15692 where
it was not possible to specify the colors for boxes in
tf.image.draw_bounding_boxes. Instead, a predefined
fixed color table was used to cycle through colors.

This fix adds colors Input to DrawBoundingBoxexV2
so that it is possible to specify the color. In case
no color is specified, the default color table will
be used.

Since there is an API change, the op is labeled as V2.

This fix fixes #15692.

Signed-off-by: Yong Tang yong.tang.github@outlook.com

jaingaurav

Thanks for the PR. Any reason why we should make colors an input argument vs an attribute? Is it useful to send the colors as a list of tensors? Advantage of using an attribute is we won't need to add a new V2 op. Using an attribute might also help avoid the copying of the color vector in draw_bounding_box_op.cc.

yongtang · 2019-01-08T21:06:52Z

@asimshankar @jaingaurav I take a look at the colors field, It is actually a list of RGBA colors so it is 2-D. Not sure it would be suitable for Attr. Should this be wrapped into 1-D (4 * colors) instead?

asimshankar · 2019-01-09T01:10:04Z

Attribute values can be tensors as well (though the only real example of that is the Const op), so you could still use an attribute.

Now, whether this should be an attribute or an input pretty much depends on whether the list of colors is something that is computed in the graph or is static. The former is certainly more general.

Shouldn't this PR also set things up so that tf.image.draw_bounding_boxes has an colors argument?

asimshankar · 2019-01-09T21:07:41Z

(From API review): Using an input seems appropriate for this. So, please go ahead and update tf.image.draw_bounding_boxes so that it uses the V2 op if there say a non-None colors argument or something like that. Thanks @yongtang !

yongtang · 2019-01-10T18:35:31Z

@asimshankar Thanks for the review. The PR has been updated with v1 pointing to old behavior and v2 allows colors input. Let me know if this is the expected implementation.

asimshankar

Apologies for the late reply, some suggestions.

asimshankar · 2019-01-23T19:09:10Z

tensorflow/core/kernels/draw_bounding_box_op.cc

        {1, 1, 0, 1},     {0, 0, 1, 1},     {1, 0, 0, 1},   {0, 1, 0, 1},
        {0.5, 0, 0.5, 1}, {0.5, 0.5, 0, 1}, {0.5, 0, 0, 1}, {0, 0, 0.5, 1},
        {0, 1, 1, 1},     {1, 0, 1, 1},
    };

+    std::vector<std::vector<float>> color_table;
+    for (int64 i = 0; i < default_color_table_length; i++) {


Instead of filling this out and then clearing it when context->num_inputs() == 3, how about doing something like this:

Define a utility function to create the default color table

namespace { std::vector<std::vector<float>> DefaultColorTable(int depth) { std::vector<std::vector<float>> color_table; color_table.emplace_back({1, 1, 0, 1}); // yellow color_table.emplace_back({0, 0, 1, 1}); // blue ... if (depth == 1) { ... } return color_table; } } // namespace

Change the kernel implementation to use the default table only if needed:

std::vector<std::vector<float>> color_table; if (context->num_inputs() == 3) { ... } if (color_table.empty()) { color_table = DefaultColorTable(depth); }

Thanks. Done.

asimshankar · 2019-01-23T19:10:03Z

tensorflow/core/kernels/draw_bounding_box_op.cc

+      OP_REQUIRES(context, colors_tensor.shape().dims() == 2,
+                  errors::InvalidArgument("colors must be a 2-D matrix",
+                                          colors_tensor.shape().DebugString()));
+      OP_REQUIRES(context, colors_tensor.shape().dim_size(1) == 4,


Does it make sense to require RGBA when depth is 1?

Thanks. The check has been updated. Now it requires the number of channels >= needed channels.

asimshankar · 2019-01-23T19:12:27Z

tensorflow/python/kernel_tests/draw_bounding_box_op_test.py

-                              [0, 1, 0, 1], [0.5, 0, 0.5, 1], [0.5, 0.5, 0, 1],
-                              [0.5, 0, 0, 1], [0, 0, 0.5, 1], [0, 1, 1, 1],
-                              [1, 0, 1, 1]])
+    default_color_table = np.asarray([[1, 1, 0, 1], [0, 0, 1, 1],


How about:

if colors is None: # THIS TABLE MUST MATCH draw_bounding_box_op.cc colors = np.asarray(...)

Done. Thanks.

asimshankar · 2019-01-23T22:29:06Z

tensorflow/python/kernel_tests/draw_bounding_box_op_test.py

@@ -85,9 +90,12 @@ def _testDrawBoundingBoxColorCycling(self, img):
      image = ops.convert_to_tensor(image)
      image = image_ops_impl.convert_image_dtype(image, dtypes.float32)
      image = array_ops.expand_dims(image, 0)
-      image = image_ops.draw_bounding_boxes(image, bboxes)
+      if colors is None:


This shouldn't be needed. We should be testing the draw_bounding_boxes function exposed in the API instead of testing the generated wrappers. So, can't this be:

image = image_ops.draw_bounding_boxes(image, bboxes, colors=colors_arg)

?

Where colors_arg points to the colors value provided to _testBoundingBoxColorCycling (i.e., before it was set to default_color_table to generate test_drawn_image?

asimshankar · 2019-01-23T22:33:30Z

tensorflow/python/ops/image_ops_impl.py

+  return gen_image_ops.draw_bounding_boxes(images, boxes, name)
+
+@tf_export('image.draw_bounding_boxes', v1=[])
+def draw_bounding_boxes_v2(


Might as well keep the colors functionality in both V1 and V2, no?
Something like:

@tf_export('image.draw_bounding_boxes', v1=[]) def draw_bounding_boxes_v2(images, boxes, colors=None, name=None): if colors is None and not compat.forward_compatible(2019, 05, 01): return gen_image_ops.draw_bounding_boxes(images, boxes, name) return gen_image_ops.draw_bounding_boxes_v2(images, boxes, colors, name) @tf_export(v1=['image.draw_bounding_boxes']) def draw_bounding_boxes(images, boxes, name=None, colors=None): return draw_bounding_boxes_v2(images, boxes, colors, name)

We don't need separate wrappers for each of the generated functions.
Thoughts?

Where compat.forward_compatible is from:

tensorflow/tensorflow/python/compat/compat.py

Line 34 in 2f2503d

def forward_compatible(year, month, day):

yongtang · 2019-01-28T20:19:41Z

Thanks @asimshankar for the review. The PR has been updated. Please take a look.

Harshini-Gadige · 2019-01-29T20:42:42Z

@yongtang Could you please look into the merging conflicts it has. Thanks !

Signed-off-by: Yong Tang <yong.tang.github@outlook.com>

yongtang · 2019-03-11T23:31:27Z

Ping @hgadig to take a look. I rebased the PR as otherwise there is a merge conflict. Since the PR has been approved, wondering if there is anything else from that needs to be done? Would like to see this PR land soon.

Harshini-Gadige · 2019-03-12T01:26:29Z

Ping @hgadig to take a look. I rebased the PR as otherwise there is a merge conflict. Since the PR has been approved, wondering if there is anything else from that needs to be done? Would like to see this PR land soon.

I do not see any approved(green tick mark) sign yet. Unless it is shown, I cannot proceed with the next steps.

yongtang · 2019-03-12T19:15:22Z

Thanks @hgadig for the help. I think @alextp already approved at one point. However, since there were merge conflicts, I rebased and pushed the PR several times. With the rebase, the approval comments were overridden.

@alextp Could you take a look to see if everything is ok, and re-approve if it is fine?

tensorflowbutler · 2019-03-27T13:00:30Z

Nagging Reviewer @jaingaurav, @asimshankar: You have been added as a reviewer to this pull request. Please add your review or reassign. It has been 14 days with no activity and the awaiting review label has been applied.

Signed-off-by: Yong Tang <yong.tang.github@outlook.com>

yongtang · 2019-03-28T18:05:04Z

/cc @alextp to take a look. I rebased and updated PR of the last commit, as there were some changes in compat test recently.

I think now all tests pass. The sanity test failed but it is unrelated and is being addressed in PR #27226.

PiperOrigin-RevId: 242188454

googlebot added the cla: yes label Dec 11, 2018

Harshini-Gadige requested a review from jaingaurav December 11, 2018 21:42

Harshini-Gadige self-assigned this Dec 11, 2018

Harshini-Gadige added the awaiting review Pull request awaiting review label Dec 11, 2018

dksb added this to Assigned Reviewer / Pending Review in PR Queue Dec 20, 2018

dksb added the size:M CL Change Size: Medium label Dec 21, 2018

jaingaurav reviewed Jan 7, 2019

View reviewed changes

jaingaurav requested a review from asimshankar January 7, 2019 17:20

tensorflowbutler removed the awaiting review Pull request awaiting review label Jan 8, 2019

asimshankar added the API review API Review label Jan 9, 2019

yongtang added kokoro:run kokoro:force-run Tests on submitted change labels Jan 10, 2019

kokoro-team removed kokoro:run kokoro:force-run Tests on submitted change labels Jan 10, 2019

asimshankar suggested changes Jan 23, 2019

View reviewed changes

PR Queue automation moved this from Assigned Reviewer to Reviewer Requested Changes Jan 23, 2019

PR Queue automation moved this from Reviewer Requested Changes to Approved by Reviewer Jan 29, 2019

alextp previously approved these changes Jan 29, 2019

View reviewed changes

Harshini-Gadige added kokoro:force-run Tests on submitted change awaiting testing (then merge) labels Jan 29, 2019

kokoro-team removed the kokoro:force-run Tests on submitted change label Jan 29, 2019

Harshini-Gadige added the stat:awaiting response Status - Awaiting response from author label Jan 29, 2019

yongtang dismissed alextp’s stale review via 7ed8427 January 29, 2019 20:55

yongtang force-pushed the 15692-draw_bounding_boxes-colors branch from 26a231a to 7ed8427 Compare January 29, 2019 20:55

yongtang added 5 commits March 11, 2019 18:20

Hide draw_bounding_boxes_v2 first

cc576db

Signed-off-by: Yong Tang <yong.tang.github@outlook.com>

Update to expose image.draw_bounding_boxes in v1 and v2 differently

681e2e6

Signed-off-by: Yong Tang <yong.tang.github@outlook.com>

Keep the colors functionality for both v1 and v2

ade7691

Signed-off-by: Yong Tang <yong.tang.github@outlook.com>

Use DefaultColorTable utility function

d43bda0

Signed-off-by: Yong Tang <yong.tang.github@outlook.com>

Fix pylint error of Unused gen_image_ops imported

8e87568

Signed-off-by: Yong Tang <yong.tang.github@outlook.com>

yongtang force-pushed the 15692-draw_bounding_boxes-colors branch from 11fdea9 to 6629afb Compare March 11, 2019 22:51

yongtang added the kokoro:force-run Tests on submitted change label Mar 27, 2019

kokoro-team removed the kokoro:force-run Tests on submitted change label Mar 27, 2019

yongtang added the kokoro:force-run Tests on submitted change label Mar 27, 2019

kokoro-team removed the kokoro:force-run Tests on submitted change label Mar 27, 2019

Fix api compat issue

fbcc59c

Signed-off-by: Yong Tang <yong.tang.github@outlook.com>

yongtang force-pushed the 15692-draw_bounding_boxes-colors branch from 6629afb to fbcc59c Compare March 28, 2019 01:54

yongtang requested a review from alextp March 28, 2019 18:02

PR Queue automation moved this from Reviewer Requested Changes to Approved by Reviewer Mar 28, 2019

alextp approved these changes Mar 28, 2019

View reviewed changes

tensorflowbutler removed the awaiting review Pull request awaiting review label Mar 29, 2019

yongtang added the kokoro:force-run Tests on submitted change label Mar 30, 2019

yongtang assigned rthadur Mar 30, 2019

kokoro-team removed kokoro:force-run Tests on submitted change labels Mar 30, 2019

rthadur unassigned Harshini-Gadige Mar 30, 2019

tensorflow-copybara merged commit fbcc59c into tensorflow:master Apr 5, 2019

PR Queue automation moved this from Approved by Reviewer to Merged Apr 5, 2019

tensorflow-copybara pushed a commit that referenced this pull request Apr 5, 2019

Merge pull request #24282 from yongtang:15692-draw_bounding_boxes-colors

e3eec7a

PiperOrigin-RevId: 242188454

yongtang deleted the 15692-draw_bounding_boxes-colors branch April 5, 2019 21:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `colors` support for `tf.image.draw_bounding_boxes` #24282

Add `colors` support for `tf.image.draw_bounding_boxes` #24282

yongtang commented Dec 11, 2018

jaingaurav left a comment

yongtang commented Jan 8, 2019

asimshankar commented Jan 9, 2019

asimshankar commented Jan 9, 2019

yongtang commented Jan 10, 2019

asimshankar left a comment

asimshankar Jan 23, 2019

yongtang Jan 28, 2019

asimshankar Jan 23, 2019

yongtang Jan 28, 2019

asimshankar Jan 23, 2019

yongtang Jan 28, 2019

asimshankar Jan 23, 2019

yongtang Jan 28, 2019

asimshankar Jan 23, 2019

yongtang Jan 28, 2019

yongtang commented Jan 28, 2019

Harshini-Gadige commented Jan 29, 2019

yongtang commented Mar 11, 2019

Harshini-Gadige commented Mar 12, 2019

yongtang commented Mar 12, 2019

tensorflowbutler commented Mar 27, 2019

yongtang commented Mar 28, 2019

Add colors support for tf.image.draw_bounding_boxes #24282

Add colors support for tf.image.draw_bounding_boxes #24282

Conversation

yongtang commented Dec 11, 2018

jaingaurav left a comment

Choose a reason for hiding this comment

yongtang commented Jan 8, 2019

asimshankar commented Jan 9, 2019

asimshankar commented Jan 9, 2019

yongtang commented Jan 10, 2019

asimshankar 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

yongtang commented Jan 28, 2019

Harshini-Gadige commented Jan 29, 2019

yongtang commented Mar 11, 2019

Harshini-Gadige commented Mar 12, 2019

yongtang commented Mar 12, 2019

tensorflowbutler commented Mar 27, 2019

yongtang commented Mar 28, 2019

Add `colors` support for `tf.image.draw_bounding_boxes` #24282

Add `colors` support for `tf.image.draw_bounding_boxes` #24282