Skip to content

fix color in draw_segmentation_masks#7520

Merged
pmeier merged 14 commits into
pytorch:mainfrom
rizavelioglu:main
Apr 21, 2023
Merged

fix color in draw_segmentation_masks#7520
pmeier merged 14 commits into
pytorch:mainfrom
rizavelioglu:main

Conversation

@rizavelioglu

Copy link
Copy Markdown
Contributor

Reproduce bug:

import torch
import torchvision.utils as utils

num_masks, h, w = 4, 100, 100
img = torch.randint(0, 256, size=(3, h, w), dtype=torch.uint8)
masks = torch.randint(0, 2, (num_masks, h, w), dtype=torch.bool)

utils.draw_segmentation_masks(img, masks, colors="red")

which throws:

ValueError: There are more masks (4) than colors (3)

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=3 and colors="red",
  • num_masks=4 and colors="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:

num_masks, h, w = 2, 100, 100

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_masks such that it also accepts a string of colors. The code was adapted similarly to how it is handled in draw_bounding_boxes, where the issue does not appear.
Specific tests passed with:

pytest test/test_utils.py -vvv -k test_draw_segmentation_masks

@pytorch-bot

pytorch-bot Bot commented Apr 17, 2023

Copy link
Copy Markdown

🔗 Helpful Links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/vision/7520

Note: Links to docs will display an error until the docs builds have been completed.

❗ 1 Active SEVs

There are 1 currently active SEVs. If your PR is affected, please view them below:

❌ 25 Failures

As of commit f7fd5d7:

NEW FAILURES - The following jobs have failed:

This comment was automatically generated by Dr. CI and updates every 15 minutes.

@pmeier pmeier left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Comment thread test/test_utils.py
Comment thread torchvision/utils.py
@rizavelioglu

Copy link
Copy Markdown
Contributor Author

Thank you @pmeier for your message!
I applied the changes and added the _parse_colors function. but with an additional argument: output_format.
I did so because draw_bounding_boxes expects a list of tuple, whereas draw_segmentation_masks expects a list of torch.tensor. What do you think?

@pmeier pmeier left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Answered your question inline. Looking good so far!

Comment thread torchvision/utils.py Outdated
Comment thread torchvision/utils.py Outdated
Comment thread test/test_utils.py Outdated
Comment thread test/test_utils.py Outdated
Comment thread torchvision/utils.py Outdated
Comment thread torchvision/utils.py Outdated
Comment thread torchvision/utils.py Outdated
Comment thread torchvision/utils.py Outdated
rizavelioglu and others added 6 commits April 18, 2023 13:43
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 pmeier left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only minor things left. Thanks a lot @rizavelioglu! LGTM if relevant CI is green.

Comment thread torchvision/utils.py Outdated
Comment thread torchvision/utils.py Outdated
Comment thread test/test_utils.py Outdated
rizavelioglu and others added 3 commits April 18, 2023 18:57
Apply suggestions from code review

Co-authored-by: Philip Meier <github.pmeier@posteo.de>

@pmeier pmeier left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread torchvision/utils.py Outdated
Comment thread torchvision/utils.py
@rizavelioglu

Copy link
Copy Markdown
Contributor Author

Thanks @pmeier, been waiting for your reply :)
Sure, I will apply the changes.

rizavelioglu and others added 3 commits April 20, 2023 19:53
remove duplicate line in docstring

Co-authored-by: Philip Meier <github.pmeier@posteo.de>
@pmeier pmeier merged commit 2925df7 into pytorch:main Apr 21, 2023
facebook-github-bot pushed a commit that referenced this pull request Apr 26, 2023
Summary: Co-authored-by: Philip Meier <github.pmeier@posteo.de>

Reviewed By: vmoens

Differential Revision: D45183663

fbshipit-source-id: db9257a53046f50c759014ab481968b5a6a5d224
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants