-
Notifications
You must be signed in to change notification settings - Fork 7.2k
[NOMERGE] Feedback Transforms API implementation #5375
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
Changes from all commits
ee0c585
8e83955
5759e50
41365f5
c5b25c8
277d221
3594f9b
dc7d0d2
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -3,15 +3,15 @@ | |
| import torch | ||
| from torchvision.prototype.features import BoundingBoxFormat | ||
| from torchvision.transforms import ( # noqa: F401 | ||
| functional as _F, | ||
| functional as _F, # Shouldn't we importing `functional_tensor`? | ||
| InterpolationMode, | ||
| ) | ||
|
|
||
| from ._meta_conversion import convert_bounding_box_format | ||
|
|
||
|
|
||
| def horizontal_flip_image(image: torch.Tensor) -> torch.Tensor: | ||
| return image.flip((-1,)) | ||
| return image.flip((-1,)) # Why not use the _F.hflip()? | ||
pmeier marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
|
|
||
| def horizontal_flip_bounding_box(bounding_box: torch.Tensor, *, image_size: Tuple[int, int]) -> torch.Tensor: | ||
|
|
@@ -20,15 +20,15 @@ def horizontal_flip_bounding_box(bounding_box: torch.Tensor, *, image_size: Tupl | |
| old_format=BoundingBoxFormat.XYXY, | ||
| new_format=BoundingBoxFormat.XYWH, | ||
| ).unbind(-1) | ||
| x = image_size[1] - (x + w) | ||
| x = image_size[1] - (x + w) # Why not avoid formatting and do straight `boxes[:, [0, 2]] = width - boxes[:, [2, 0]]`? | ||
pmeier marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| return convert_bounding_box_format( | ||
| torch.stack((x, y, w, h), dim=-1), | ||
| old_format=BoundingBoxFormat.XYWH, | ||
| new_format=BoundingBoxFormat.XYXY, | ||
| ) | ||
|
|
||
|
|
||
| _resize_image = _F.resize | ||
| _resize_image = _F.resize # How about videos? The low level transfroms supports it | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Right now there is no custom feature type for videos and the tentatively plan is to treat videos as
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We should finalize this plan soon. We got outstanding work with Videos so we need to be able to have an idea of how this works for them. Once the plan is finalized, we will be able to parallelize the work of porting all transforms concurrently. I agree about the underlying kernels being agnostic, sounds good.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Annoyingly, I still haven't gotten a reply from any of them.
Is there a downside to creating a
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @bjuncek ping me offline with the names of the people you try to talk to to see if I can help. When we say "extra branch dimensions", could you clarify the proposal?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think there is real downside other than being more verbose in our transforms implementations. For example, we would need to have |
||
|
|
||
|
|
||
| def resize_image( | ||
|
|
@@ -40,7 +40,7 @@ def resize_image( | |
| ) -> torch.Tensor: | ||
| new_height, new_width = size | ||
| num_channels, old_height, old_width = image.shape[-3:] | ||
| batch_shape = image.shape[:-3] | ||
| batch_shape = image.shape[:-3] # In some places you use `image` to denote batches and in others `image_batch`. Should we align the names? | ||
datumbox marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| return _resize_image( | ||
| image.reshape((-1, num_channels, old_height, old_width)), | ||
| size=size, | ||
|
|
@@ -71,7 +71,7 @@ def resize_bounding_box( | |
| ) -> torch.Tensor: | ||
| old_height, old_width = old_image_size | ||
| new_height, new_width = new_image_size | ||
| return ( | ||
| return ( # Shouldn't we be using a low-level kernel instead, similar to above? | ||
pmeier marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| bounding_box.view(-1, 2, 2) | ||
| .mul(torch.tensor([new_width / old_width, new_height / old_height])) | ||
| .view(bounding_box.shape) | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.