[prototype] Minor improvements on functional #6832
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
datumbox
commented
Oct 25, 2022
Comment on lines
-213
to
-214
| if not (isinstance(image, torch.Tensor)): | ||
| raise TypeError("Input img should be Tensor image") |
There was a problem hiding this comment.
As decided previously, removing the final tensor checks on tensor kernels.
| @@ -183,12 +183,8 @@ def clamp_bounding_box( | |||
| return convert_format_bounding_box(xyxy_boxes, BoundingBoxFormat.XYXY, format) | |||
|
|
|||
|
|
|||
| def _split_alpha(image: torch.Tensor) -> Tuple[torch.Tensor, torch.Tensor]: | |||
| return image[..., :-1, :, :], image[..., -1:, :, :] | |||
There was a problem hiding this comment.
tensor_split is marginally faster because it avoids the double indexing:
[------------------------------- Convert_color_space_image_tensor cpu torch.uint8 ------------------------------]
| convert_color_space_image_tensor v2 before | convert_color_space_image_tensor_ v2 after
1 threads: ------------------------------------------------------------------------------------------------------
(4, 400, 400) | 217 | 213
6 threads: ------------------------------------------------------------------------------------------------------
(4, 400, 400) | 297 | 292
Times are in microseconds (us).
[------------------------------ Convert_color_space_image_tensor cuda torch.uint8 ------------------------------]
| convert_color_space_image_tensor v2 before | convert_color_space_image_tensor_ v2 after
1 threads: ------------------------------------------------------------------------------------------------------
(4, 400, 400) | 56.4 | 52
6 threads: ------------------------------------------------------------------------------------------------------
(4, 400, 400) | 56.4 | 52
Times are in microseconds (us).
[------------------------------ Convert_color_space_image_tensor cpu torch.float32 -----------------------------]
| convert_color_space_image_tensor v2 before | convert_color_space_image_tensor_ v2 after
1 threads: ------------------------------------------------------------------------------------------------------
(4, 400, 400) | 236 | 232
6 threads: ------------------------------------------------------------------------------------------------------
(4, 400, 400) | 325 | 321
Times are in microseconds (us).
[----------------------------- Convert_color_space_image_tensor cuda torch.float32 -----------------------------]
| convert_color_space_image_tensor v2 before | convert_color_space_image_tensor_ v2 after
1 threads: ------------------------------------------------------------------------------------------------------
(4, 400, 400) | 56.9 | 52.4
6 threads: ------------------------------------------------------------------------------------------------------
(4, 400, 400) | 56.7 | 52.4
Times are in microseconds (us).
| def _strip_alpha(image: torch.Tensor) -> torch.Tensor: | ||
| image, alpha = _split_alpha(image) | ||
| image, alpha = torch.tensor_split(image, indices=(-1,), dim=-3) |
There was a problem hiding this comment.
IMO we don't need a private one liner method for something Torch has a built in method.
There was a problem hiding this comment.
Non-blocking: I would prefer it since it is a little more expressive. Plus, since we have a _add_alpha function it would be consistent to have the complementary _split_alpha as well. With this change we have three identical, non-trivial calls of torch.tensor_split. At a glance, it is harder to see that these three actually do exactly the same.
There was a problem hiding this comment.
OK I'll restore `_split_alpha.
There was a problem hiding this comment.
Restoring this causes more harm than good. JIT is complaining that it's not longer a tuple, which means I would have to split into 2 lines and return as tuple to please it. I don't think it's worth this.
31 tasks
Closed
facebook-github-bot
pushed a commit
that referenced
this pull request
Oct 27, 2022
Summary: * Minor improvements on functional. * Restore `_split_alpha`. * Revert "Restore `_split_alpha`." This reverts commit 2286120. Reviewed By: YosuaMichael Differential Revision: D40722902 fbshipit-source-id: 3a574939365abd1b74ed3a558b4354b1c40fc883
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Minor last improvements on the functional kernels. I consider these mostly code-quality improvements rather that speed optimizations.
cc @vfdev-5 @bjuncek @pmeier