Skip to content

add tests for F.pad_bounding_box #6038

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 6 commits into from
May 26, 2022
Merged

add tests for F.pad_bounding_box #6038

merged 6 commits into from
May 26, 2022

Conversation

pmeier
Copy link
Collaborator

@pmeier pmeier commented May 17, 2022

Related to #5514 as it is the last missing item.

@vfdev-5
Copy link
Collaborator

vfdev-5 commented May 17, 2022

@pmeier a correctness test on cpu/cuda could be good to have as well

@pmeier
Copy link
Collaborator Author

pmeier commented May 17, 2022

a correctness test on cpu/cuda could be good to have as well

Agreed, but I'm covered right now. Can we add that after #5879? Otherwise, would you be able to contribute it?

@vfdev-5
Copy link
Collaborator

vfdev-5 commented May 23, 2022

@pmeier OK let me merge this PR and add a correctness test

Copy link
Collaborator Author

@pmeier pmeier left a comment

Choose a reason for hiding this comment

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

A few minor comments. Otherwise LGTM. Thanks Victor!

Comment on lines +1140 to +1147
bbox = convert_bounding_box_format(
bbox, old_format=features.BoundingBoxFormat.XYXY, new_format=bbox_format, copy=False
)
if bbox.dtype != bbox_dtype:
# Temporary cast to original dtype
# e.g. float32 -> int
bbox = bbox.to(bbox_dtype)
return bbox
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

  1. There is no need for this if: .to is a no-op if no conversion needs to be performed.
  2. You don't need to use the dtype explicitly. You can pass a tensor to .to in which case it will convert device and dtype to the input.
Suggested change
bbox = convert_bounding_box_format(
bbox, old_format=features.BoundingBoxFormat.XYXY, new_format=bbox_format, copy=False
)
if bbox.dtype != bbox_dtype:
# Temporary cast to original dtype
# e.g. float32 -> int
bbox = bbox.to(bbox_dtype)
return bbox
return convert_bounding_box_format(
bbox, old_format=features.BoundingBoxFormat.XYXY, new_format=bbox_.format, copy=False
).to(bbox_)

Copy link
Collaborator

@vfdev-5 vfdev-5 May 26, 2022

Choose a reason for hiding this comment

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

@pmeier actually it does not work with .to between tensors and BoundingBox:

>>> t = torch.tensor([[1.0, 2.0, 3.0, 4.0]])
>>> bbox = features.BoundingBox([[0, 1, 2, 3]], format=features.BoundingBoxFormat.XYXY, image_size=(32, 32))
>>> t.to(bbox)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/vision/torchvision/prototype/features/_feature.py", line 83, in __torch_function__
    return cls.new_like(args[0], output, dtype=output.dtype, device=output.device)
  File "/vision/torchvision/prototype/features/_bounding_box.py", line 54, in new_like
    format=format if format is not None else other.format,
AttributeError: 'Tensor' object has no attribute 'format'

I'll keep my code, especially it is a temporary cast once we fixed convert_bounding_box_format we can remove it quickly

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'll added #6094 to track this.

@vfdev-5 vfdev-5 merged commit 1d50dfa into pytorch:main May 26, 2022
@pmeier pmeier deleted the test-pad-bbox branch May 26, 2022 13:39
facebook-github-bot pushed a commit that referenced this pull request Jun 1, 2022
Summary:
* add tests for F.pad_bounding_box

* Added correctness tests for pad and reimplemented bbox op to keep dtype

* Update _geometry.py

Reviewed By: NicolasHug

Differential Revision: D36760926

fbshipit-source-id: 6c2a5430790e2db778911dbf62ce8e38f6e9b603

Co-authored-by: vfdev <[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.

3 participants