Skip to content

Adds fill paramter to utils.draw_bounding_boxes #3296

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 10 commits into from
Feb 1, 2021

Conversation

oke-aditya
Copy link
Contributor

Closes #3280 .

Currently, I have kept fill to Tuple[int, int, int, int] or str.
Let me know if changes are needed.

Also, let me know how we can test this.

Copy link
Contributor

@datumbox datumbox left a comment

Choose a reason for hiding this comment

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

Thanks for the PR! I left a couple of comments for your consideration.

Also could you also change the unit-test to support the new param once you finalize the implementation? You will need to produce a new expected file but I think the code already supports this if you delete the old file.

txt_font = ImageFont.load_default() if font is None else ImageFont.truetype(font=font, size=font_size)

for i, bbox in enumerate(img_boxes):
color = None if colors is None else colors[i]
draw.rectangle(bbox, width=width, outline=color)
if fill:
fill_color = color + [100]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think we would need to set alpha channel to some hardcoded value. Since that gives the semi transparent look.

Copy link
Contributor

Choose a reason for hiding this comment

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

I just noticed that this causes issues on the type checks. Also this is not going to work for all values of color. There are various corner cases:

  1. The color could be None
  2. It could be a string
  3. Or a tuple.

It might be worth confirming that all work as expected by adding a test, what you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If fill is true

If color is None lets fill with default color.

If color is Tuple[int, int, int] I think we just add alpha of 100 as above and do.

If color is str I think we need to get RGB value of the str add default alpha of 100 and draw ?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think we need to handle the fill_color = color + [100] especially in each corner case:

>>> None + [100]
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: unsupported operand type(s) for +: 'NoneType' and 'list'

>>> (0, 0, 0) + [100]
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: can only concatenate tuple (not "list") to tuple

>>> "red" + [100]
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: can only concatenate str (not "list") to str

Copy link
Contributor Author

@oke-aditya oke-aditya Jan 27, 2021

Choose a reason for hiding this comment

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

Sure. Sorry, I'm bit low on bandwidth this week but will complete by next week.
I will add tests too to check these corner cases. It's really simple to handle and I should have thought before 😅

Copy link
Contributor

Choose a reason for hiding this comment

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

Don't worry mate. :) Take your time! Ping me when ready to make sure I review it again. Thanks!

Copy link
Contributor

@datumbox datumbox left a comment

Choose a reason for hiding this comment

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

Would you be able to update the following test to use your new feature?

def test_draw_boxes(self):
img = torch.full((3, 100, 100), 255, dtype=torch.uint8)
boxes = torch.tensor([[0, 0, 20, 20], [0, 0, 0, 0],
[10, 15, 30, 35], [23, 35, 93, 95]], dtype=torch.float)
labels = ["a", "b", "c", "d"]
colors = ["green", "#FF00FF", (0, 255, 0), "red"]
result = utils.draw_bounding_boxes(img, boxes, labels=labels, colors=colors)
path = os.path.join(os.path.dirname(os.path.abspath(__file__)), "assets", "fakedata", "draw_boxes_util.png")
if not os.path.exists(path):
res = Image.fromarray(result.permute(1, 2, 0).contiguous().numpy())
res.save(path)
expected = torch.as_tensor(np.array(Image.open(path))).permute(2, 0, 1)
self.assertTrue(torch.equal(result, expected))

Feel free to update the expected file as well.

@datumbox datumbox self-requested a review January 27, 2021 14:38
Copy link
Contributor

@datumbox datumbox left a comment

Choose a reason for hiding this comment

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

@oke-aditya I added a couple of more comments, please let me know what you think.

@datumbox datumbox marked this pull request as draft January 27, 2021 14:44
Copy link
Contributor

@datumbox datumbox left a comment

Choose a reason for hiding this comment

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

.

@oke-aditya
Copy link
Contributor Author

oke-aditya commented Jan 30, 2021

Done ! @datumbox It works well now, semi transparent images look good.
I just modified the test and the expected file.
Which should now run with filled option. Since it covers all cases it should be fine.

And sorry for the delay, it was really simple to fix, but I was on vacation 😅

@oke-aditya oke-aditya marked this pull request as ready for review January 30, 2021 18:02
@oke-aditya oke-aditya requested a review from datumbox January 30, 2021 18:02
@codecov
Copy link

codecov bot commented Jan 30, 2021

Codecov Report

Merging #3296 (6134881) into master (1e3a183) will decrease coverage by 0.06%.
The diff coverage is 43.75%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3296      +/-   ##
==========================================
- Coverage   73.96%   73.90%   -0.07%     
==========================================
  Files         104      104              
  Lines        9607     9618      +11     
  Branches     1537     1543       +6     
==========================================
+ Hits         7106     7108       +2     
- Misses       2024     2028       +4     
- Partials      477      482       +5     
Impacted Files Coverage Δ
torchvision/utils.py 59.57% <43.75%> (-5.49%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1e3a183...e28d6cc. Read the comment docs.

Copy link
Contributor

@datumbox datumbox left a comment

Choose a reason for hiding this comment

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

LGTM, thanks a lot for the PR @oke-aditya!

@datumbox datumbox merged commit b2cf604 into pytorch:master Feb 1, 2021
@oke-aditya oke-aditya deleted the add_fill branch February 1, 2021 11:53
facebook-github-bot pushed a commit that referenced this pull request Feb 4, 2021
Summary:
* adds fill paramter

* small doc edit

* Change fill to bool

* add filled

* fix the bugs

* Fixes bugs

* adds test with fill param

* fix tuple bug

Reviewed By: datumbox

Differential Revision: D26226612

fbshipit-source-id: 485e9aec56c4f92da64c37a080ebcb6a2182a76f

Co-authored-by: Vasilis Vryniotis <[email protected]>
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.

Add fill parameter to utils.draw_bounding_boxes.
3 participants