Add experimental.tensor_resize operator #4492
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
jantonguirao
force-pushed
the
tensor_resize
branch
3 times, most recently
from
909a166 to
282f150
Compare
jantonguirao
force-pushed
the
tensor_resize
branch
from
efdc333 to
0d4ebc3
Compare
18 tasks
jantonguirao
force-pushed
the
tensor_resize
branch
from
cab83f9 to
ceb224d
Compare
jantonguirao
changed the title
mzient
reviewed
Jan 4, 2023
Comment on lines
110
to
119
| } else if (alignment[d] == -1) { | ||
| // keep start of the ROI | ||
| center = params.src_lo[d]; | ||
| } else if (alignment[d] == 1) { | ||
| // keep end of the ROI | ||
| center = params.src_hi[d]; | ||
| } else { | ||
| DALI_FAIL(make_string("Unsupported alignment value ", alignment[d], | ||
| ". Supported values are 0, -1, 1")); | ||
| } |
There was a problem hiding this comment.
To consider:
Suggested change
| } else if (alignment[d] == -1) { | |
| // keep start of the ROI | |
| center = params.src_lo[d]; | |
| } else if (alignment[d] == 1) { | |
| // keep end of the ROI | |
| center = params.src_hi[d]; | |
| } else { | |
| DALI_FAIL(make_string("Unsupported alignment value ", alignment[d], | |
| ". Supported values are 0, -1, 1")); | |
| } | |
| } else if (alignment[d] < 0) { | |
| // keep start of the ROI | |
| center = params.src_lo[d]; | |
| } else if (alignment[d] > 0) { | |
| // keep end of the ROI | |
| center = params.src_hi[d]; | |
| } |
Having a sign (as opposed to exact value) define the behavior may simplify the client code. The usage of sign is not uncommon (see strcmp or the "spaceship" operator <=>).
There was a problem hiding this comment.
A random thought: how about having an alignment as a float 0..1? 0.5 would denote the center.
center = (1 - alignment[d]) * params.src_lo[d] + alignment[d] * params.src_hi[d]We could even skip the range check and have the invariant point anywhere - possibly outside the ROI.
mzient
reviewed
Jan 4, 2023
Comment on lines
43
to
45
| Accepted values are -1 (align with top-left corner), 0 (centered), 1 (align with bottom-right corner). | ||
| By default, 0 (centered) is assumed. Contains as many elements as dimensions provided for sizes/scales. | ||
| If only one value is provided, it is apply to all dimensions.)code", |
There was a problem hiding this comment.
If you go with the sign, adjust the comment accordingly.
mzient
reviewed
Jan 4, 2023
Comment on lines
40
to
42
| .AddOptionalArg<int>("alignment", R"code(Determines the position of the ROI | ||
| when using scales (provided or calculated). |
There was a problem hiding this comment.
I'd add some more explanation.
The real output size must be integer and may differ from "ideal" output size calculated as
input (or ROI) size multiplied by the scale factor. In that case, the output size is rounded
(according to `size_rounding` policy) and the input ROI needs to be adjusted to maintain
the scale factor. This parameter defines which point of the ROI should remain constant.
mzient
reviewed
Jan 4, 2023
| * | ``"round"`` - Rounds the resulting size to the nearest integer value, with halfway cases rounded away from zero. | ||
| * | ``"truncate"`` - Discards the fractional part of the resulting size. | ||
| * | ``"ceil"`` - Rounds up the resulting size to the next integer value.)code", | ||
| "truncate") |
There was a problem hiding this comment.
why not "round"? Don't we round in normal Resize?
There was a problem hiding this comment.
true, by default we round. I'll change it
jantonguirao
force-pushed
the
tensor_resize
branch
from
58fe01d to
410419d
Compare
mzient
reviewed
Jan 4, 2023
Comment on lines
55
to
56
| Contains as many elements as dimensions provided for sizes/scales. If only one value is provided, it is | ||
| applied to all dimensions.)code", |
There was a problem hiding this comment.
Now, with multiple paragraphs above, it's hard to infer the subject.
Suggested change
| Contains as many elements as dimensions provided for sizes/scales. If only one value is provided, it is | |
| applied to all dimensions.)code", | |
| The value of this argument contains as many elements as dimensions provided for | |
| sizes/scales. If only one value is provided, it is applied to all dimensions.)code", |
mzient
reviewed
Jan 4, 2023
|
|
||
| This point is calculated as ``center = (1 - alignment) * roi_start + alignment * roi_end``. | ||
| Alignment 0.0 denotes alignment with the start of the ROI, 0.5 with the center of the region, and 1.0 with the end. | ||
| Note that when ROI is not specified, roi_start=0 and roi_end=size is assumed. |
There was a problem hiding this comment.
Suggested change
| Note that when ROI is not specified, roi_start=0 and roi_end=size is assumed. | |
| Note that when ROI is not specified, roi_start=0 and roi_end=input_size is assumed. |
mzient
reviewed
Jan 4, 2023
stiepan
reviewed
Jan 4, 2023
stiepan
reviewed
Jan 4, 2023
stiepan
reviewed
Jan 4, 2023
stiepan
reviewed
Jan 4, 2023
stiepan
reviewed
Jan 4, 2023
stiepan
reviewed
Jan 5, 2023
stiepan
reviewed
Jan 5, 2023
stiepan
reviewed
Jan 5, 2023
jantonguirao
force-pushed
the
tensor_resize
branch
from
8b98876 to
cdad671
Compare
stiepan
reviewed
Jan 11, 2023
Signed-off-by: Joaquin Anton <janton@nvidia.com>
Signed-off-by: Joaquin Anton <janton@nvidia.com>
jantonguirao
force-pushed
the
tensor_resize
branch
from
1dd4410 to
e59ca1f
Compare
|
!build |
|
CI MESSAGE: [7083752]: BUILD STARTED |
|
CI MESSAGE: [7083752]: BUILD FAILED |
mzient
reviewed
Jan 24, 2023
| first_spatial_dim_ = 0; | ||
| spatial_ndim_ = orig_spatial_ndim + add_leading_spatial_ndim_; | ||
| expand_dims(expanded_input_shape_, input_shape, first_spatial_dim_, spatial_ndim_); | ||
| (void) input_shape; // should use expanded_input_shape_ from now on |
There was a problem hiding this comment.
What is this line for? Does the compiler issue a warning when a variable is used after a void cast or sth? If not, leave just the comment.
There was a problem hiding this comment.
it was more for "documentation". Removed.
mzient
reviewed
Jan 24, 2023
| return true; | ||
| }; | ||
|
|
||
| int can_trim_n = std::max(0, spatial_ndim_ - min_ndim); // at least 2 spatial dims should remain |
There was a problem hiding this comment.
Suggested change
| int can_trim_n = std::max(0, spatial_ndim_ - min_ndim); // at least 2 spatial dims should remain | |
| int can_trim_n = std::max(0, spatial_ndim_ - min_ndim); // at least min_ndim spatial dims should remain |
mzient
reviewed
Jan 24, 2023
| }; | ||
|
|
||
| // at least min_ndim should remain | ||
| int can_trim_n = std::max(0, spatial_ndim_ - min_ndim); |
There was a problem hiding this comment.
Nitpick:
can_trim_n reads like a boolean (can I trim n dimensions?)
Suggested change
| int can_trim_n = std::max(0, spatial_ndim_ - min_ndim); | |
| int ndim_to_trim = std::max(0, spatial_ndim_ - min_ndim); |
mzient
approved these changes
Jan 24, 2023
jantonguirao
force-pushed
the
tensor_resize
branch
from
70a6a5b to
0e13f7d
Compare
Signed-off-by: Joaquin Anton <janton@nvidia.com>
jantonguirao
force-pushed
the
tensor_resize
branch
from
0e13f7d to
f85f934
Compare
|
!build |
|
CI MESSAGE: [7093218]: BUILD STARTED |
|
!build |
|
CI MESSAGE: [7093767]: BUILD STARTED |
jantonguirao
force-pushed
the
tensor_resize
branch
from
80f4ba7 to
ae7e5cb
Compare
Signed-off-by: Joaquin Anton <janton@nvidia.com>
jantonguirao
force-pushed
the
tensor_resize
branch
from
ae7e5cb to
39fffa0
Compare
|
CI MESSAGE: [7095259]: BUILD FAILED |
|
CI MESSAGE: [7095769]: BUILD STARTED |
|
CI MESSAGE: [7095769]: BUILD FAILED |
|
!build |
|
CI MESSAGE: [7101791]: BUILD STARTED |
|
CI MESSAGE: [7101791]: BUILD PASSED |
aderylo
pushed a commit
to zpp-dali-2022/DALI
that referenced
this pull request
Mar 17, 2023
Introduces a layout-agnostic frontend for Resize
New operator experimental.tensor_resize
All dimensions can be resized (including channels), the implementation is able to detect unchanged dimensions at the beginning and the end, and treat those as non-spatial dimensions.
Up to 3 spatial dimensions supported
Signed-off-by: Joaquin Anton <janton@nvidia.com>
Closed
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.
Signed-off-by: Joaquin Anton janton@nvidia.com
Category:
New feature
Description:
experimental.tensor_resizeAdditional information:
Affected modules and functionalities:
Key points relevant for the review:
Resize parameter logic
Tests:
test_tensor_resize.test_resize_upsample_scales_nearest
test_tensor_resize.test_resize_downsample_scales_nearest
test_tensor_resize.test_resize_upsample_sizes_nearest
test_tensor_resize.test_resize_upsample_scales_linear
test_tensor_resize.test_resize_downsample_scales_linear
test_tensor_resize.test_resize_alignment
test_tensor_resize.test_resize_upsample_scales_cubic
test_tensor_resize.test_resize_downsample_scales_cubic
test_tensor_resize.test_resize_upsample_sizes_cubic
test_tensor_resize.test_resize_downsample_sizes_cubic
Checklist
Documentation
DALI team only
Requirements
REQ IDs: N/A
JIRA TASK: DALI-3148