fix color in draw_segmentation_masks#7520
Conversation
pmeier
left a comment
There was a problem hiding this comment.
Thanks @rizavelioglu for the PR. The idea to align with the bounding box functionality SGTM. IMO, we should have a def _parse_colors(colors, num_objects: int) function that handles this. With that, we don't need to duplicate the parsing in both functions. Since it would be a "generalization" of _generate_color_palette, we could inline that into the new function. Thoughts?
|
Thank you @pmeier for your message! |
pmeier
left a comment
There was a problem hiding this comment.
Answered your question inline. Looking good so far!
No need to "guard" the start and end of the string. Co-authored-by: Philip Meier <github.pmeier@posteo.de>
The function name is expressive enough to not need the pass a keyword argument. Co-authored-by: Philip Meier <github.pmeier@posteo.de>
Reordering for better legibility. Co-authored-by: Philip Meier <github.pmeier@posteo.de>
Co-authored-by: Philip Meier <github.pmeier@posteo.de>
Co-authored-by: Philip Meier <github.pmeier@posteo.de>
pmeier
left a comment
There was a problem hiding this comment.
Only minor things left. Thanks a lot @rizavelioglu! LGTM if relevant CI is green.
Apply suggestions from code review Co-authored-by: Philip Meier <github.pmeier@posteo.de>
pmeier
left a comment
There was a problem hiding this comment.
Sorry for the delay @rizavelioglu. Two more minor things that I found in a final pass, but they are non-blocking. Let me know if you want to address them. Otherwise we can also merge as is.
|
Thanks @pmeier, been waiting for your reply :) |
remove duplicate line in docstring Co-authored-by: Philip Meier <github.pmeier@posteo.de>
Summary: Co-authored-by: Philip Meier <github.pmeier@posteo.de> Reviewed By: vmoens Differential Revision: D45183663 fbshipit-source-id: db9257a53046f50c759014ab481968b5a6a5d224
Reproduce bug:
which throws:
where the expected outcome is the drawing of all the segmentation masks in a single color (as stated in the docs).
Notice that, in the following settings the function works as intended;
num_masks=3andcolors="red",num_masks=4andcolors="blue",meaning, in the case of
num_masks=len(colors).Currently, the respective tests pass even though the function does not work properly. This is because the test is checked in the presence of only 2 masks:
vision/test/test_utils.py
Line 205 in b78d98b
Therefore, for any input whose length is larger than 2, i.e.
"red","#FF00FF",(240, 10, 157), the tests will pass.The Fix:
I have adapted the code in
draw_segmentation_maskssuch that it also accepts a string of colors. The code was adapted similarly to how it is handled indraw_bounding_boxes, where the issue does not appear.Specific tests passed with: