mark scales as optional and remove tf_half_pixel_for_nn in resize op #3026

daquexian · 2020-09-20T06:05:06Z

mark 'scales' of the resize op as an optional input (Doc update for Resize to correct some wrong information #2948 (comment)).
remove coordinate_transformation_mode 'tf_half_pixel_for_nn'. This mode was introduced by me for resize_nearest half_pixel of tensorflow, but it is indeed unnecessary.
The equation of nearest + half_pixel is x' = round((x + 0.5) / scale - 0.5), while in tf it is optimized as x' = floor((x + 0.5) / scale). As a result, I introduced a transformation mode 'tf_half_pixel_for_nn' for the transformation (x + 0.5) / scale in PR Update resize op #2057.
However, as shown above, it should be replaced with a plain 'half_pixel' + 'round_prefer_ceil'

…favor of half_pixel + round_prefer_ceil Signed-off-by: daquexian <daquexian566@gmail.com>

Signed-off-by: daquexian <daquexian566@gmail.com>

hariharans29 · 2020-10-01T22:43:27Z

docs/Operators.md

-<dt><tt>scales</tt> : tensor(float)</dt>
-<dd>The scale array along each dimension. It takes value greater than 0. If it's less than 1, it's sampling down, otherwise, it's upsampling. The number of elements of 'scales' should be the same as the rank of input 'X'. Only one of 'scales' and 'sizes' can be specified. If 'size' is specified, then set scales to empty data (zero shape) in this operator's input list.</dd>
+<dt><tt>scales</tt> (optional) : tensor(float)</dt>
+<dd>The scale array along each dimension. It takes value greater than 0. If it's less than 1, it's sampling down, otherwise, it's upsampling. The number of elements of 'scales' should be the same as the rank of input 'X'. Only one of 'scales' and 'sizes' can be specified. If 'size' is needed, the user can use an empty string as the name of 'scales' in this operator's input list.</dd>
 <dt><tt>sizes</tt> (optional) : tensor(int64)</dt>
 <dd>The size of the output tensor. The number of elements of 'sizes' should be the same as the rank of input 'X'. Only one of 'scales' and 'sizes' can be specified.</dd>


Since both scales and sizes are now "optional" per schema - should we go ahead and include a line that says - one of the two optional inputs MUST be specified. We already have a line that says "only one can be specified...", maybe we can say - "one of the two MUST be specified and it is an error if both are specified."

Also does the onnx checker catch cases where the Resize node has only two inputs - which means it is missing one of the two ? Also, Inputs (2-4) - is that correct ?

Also does the onnx checker catch cases where the Resize node has only two inputs - which means it is missing one of the two ? Also, Inputs (2-4) - is that correct ?

Agree. But it seems like the infra (checker and doc generator) cannot support "one of these optional inputs must be specified".

askhade · 2020-10-02T18:05:38Z

docs/Operators.md

@@ -15669,15 +15666,15 @@ x_original = length_resized > 1 ? start_x * (length_original - 1) + x_resized *
 <dd>Four modes: round_prefer_floor (default, as known as round half down), round_prefer_ceil (as known as round half up), floor, ceil. Only used by nearest interpolation. It indicates how to get "nearest" pixel in input tensor from x_original, so this attribute is valid only if "mode" is "nearest".</dd>
 </dl>

-#### Inputs (3 - 4)
+#### Inputs (2 - 4)

 <dl>
 <dt><tt>X</tt> : T1</dt>
 <dd>N-D tensor</dd>
 <dt><tt>roi</tt> : T2</dt>


why not make roi optional as well? since it only takes effect when coordinate_transformation_mode is "tf_crop_and_resize"

askhade · 2020-10-02T18:06:41Z

docs/Operators.md

@@ -15669,15 +15666,15 @@ x_original = length_resized > 1 ? start_x * (length_original - 1) + x_resized *
 <dd>Four modes: round_prefer_floor (default, as known as round half down), round_prefer_ceil (as known as round half up), floor, ceil. Only used by nearest interpolation. It indicates how to get "nearest" pixel in input tensor from x_original, so this attribute is valid only if "mode" is "nearest".</dd>
 </dl>

-#### Inputs (3 - 4)
+#### Inputs (2 - 4)


Should this be 2 to 3? Because Sizes and Scales should not be specified together right.

@askhade @hariharans29 I agree that here should be 2-3, but this line is generated automatically. Is there any way to overwrite it?

askhade · 2020-10-02T18:11:34Z

@daquexian : Marking this PR for 1.8 release. Let's try and get this change checked in before 1.8 to avoid another version bump for resize.

Signed-off-by: daquexian <daquexian566@gmail.com>

…_resize

daquexian · 2020-10-05T03:58:05Z

@askhade @hariharans29 I have updated the spec to "one of the two MUST be specified and it is an error if both are specified" and made the input "roi" also optional. When it comes to "input (2-4)" and the checker problem, maybe we need to update the infra to support the case "one of these optional inputs must be specified" first?

hariharans29 · 2020-10-07T19:29:29Z

@askhade @hariharans29 I have updated the spec to "one of the two MUST be specified and it is an error if both are specified" and made the input "roi" also optional. When it comes to "input (2-4)" and the checker problem, maybe we need to update the infra to support the case "one of these optional inputs must be specified" first?

Thanks for adding the line to the spec. It leaves no place for ambiguity atleast with the spec description. It does a look a little weird to see (2-4) when actually the correct line there must read (2-3), but looks like ONNX doesn't have the infrastructure to deal with "inter-dependence" of optional inputs/outputs (i.e.) we are not able to say - 'scales' and 'sizes' are individually optional both they are NOT both optional and both can't co-exist. So, I guess we can leave it for now and enhance the infrastructure for later.

For the second aspect of the PR - deprecating tf_half_pixel_for_nn- does this cause any issue for a model that is using this attribute and trying to upgrade to opset-13 ? It seems like (from your comment in the PR) that all the converter (or opset upgrader) would have to do is replace tf_half_pixel_for_nn with half_pixel' + 'round_prefer_ceil and the upgrade will be seamless. Is this correct or do you foresee any other upgrade issues faced by models already using this attribute ?

hariharans29 · 2020-10-07T19:33:59Z

docs/TestCoverage.md

@@ -9760,7 +9760,6 @@ data = np.array([[[
 ]]], dtype=np.float32)

 roi = np.array([], dtype=np.float32)
-scales = np.array([], dtype=np.float32)


should we add a test where scales exists and sizes does not ?

Thanks. There are already such tests before this PR submitted (e.g., https://github.com/onnx/onnx/blob/master/onnx/backend/test/case/node/resize.py#L341) .

hariharans29 · 2020-10-07T19:35:18Z

onnx/defs/tensor/defs.cc

        .Input(
            2,
            "scales",
            "The scale array along each dimension. It takes value greater than 0. If it's less than 1,"
            " it's sampling down, otherwise, it's upsampling. The number of elements of 'scales' should"
-            " be the same as the rank of input 'X'. Only one of 'scales' and 'sizes' can be specified. If 'size' is specified, then set scales to empty data (zero shape) in this operator's input list.",
+            " be the same as the rank of input 'X'. One of 'scales' and 'sizes' MUST be specified and it is an error if both are specified. If 'size' is needed, the user can use an empty string as the name of 'scales' in this operator's input list.",


nit: If 'sizes' is needed (not if 'size' is needed) for consistency

Thanks. Fixed

hariharans29 · 2020-10-07T19:38:59Z

Overall, I am good with this PR - left a couple of minor comments

daquexian · 2020-10-12T01:25:40Z

For the second aspect of the PR - deprecating tf_half_pixel_for_nn- does this cause any issue for a model that is using this attribute and trying to upgrade to opset-13 ? It seems like (from your comment in the PR) that all the converter (or opset upgrader) would have to do is replace tf_half_pixel_for_nn with half_pixel' + 'round_prefer_ceil and the upgrade will be seamless. Is this correct or do you foresee any other upgrade issues faced by models already using this attribute ?

@hariharans29 Yes, it is correct. All the converter only have to replace replace 'tf_half_pixel_for_nn' with 'half_pixel' + 'round_prefer_ceil'.

hariharans29

LGTM

Signed-off-by: daquexian <daquexian566@gmail.com>

askhade

@daquexian : It seems the tests were not updated after roi was made optional. Can you update the test script and regenerate the tests again. Thanks!

Signed-off-by: daquexian <daquexian566@gmail.com>

…_resize

daquexian · 2020-10-13T08:04:36Z

@daquexian : It seems the tests were not updated after roi was made optional. Can you update the test script and regenerate the tests again. Thanks!

Thanks. Updated.

askhade · 2020-10-13T16:46:20Z

@daquexian : The tests are checked in with the test script. So you need to update the generated tests as well. I think you only updated the test script.
FYI: We are cutting the release branch tomorrow morning and as much as possible I want to check this change in otherwise we will need another version bump for resize op.

gramalingam · 2020-10-13T18:46:14Z

It is true that the documentation-generator does not have infrastructure to fix the number of inputs expected. However, the type-and-shape-inference checker can be augmented to verify that exactly one of two inputs is specified. (Well, sort of ... we can do the check using both getNumInputs and getInputType; we will need to use getInputType as a proxy for whether a particular input is specified; it is not perfect, but it is reasonable. The only minor issue would be a misleading error-message in the case where an input is specified, but type-inference could not infer its type.

gramalingam · 2020-10-13T19:05:40Z

Specifically: a utility function like below should help:

   inline bool hasInput(InferenceContext& ctx, size_t n) {
       return ctx.getNumInputs() > static_cast<size_t>(n) && ctx.getInputType(n);
}

We can then add a check one of the two inputs is specified.

askhade · 2020-10-13T19:09:11Z

Shape inference code for resize needs fixes.

onnx/onnx/defs/tensor/utils.cc

Line 73 in 5305778

if (is_resize_op && ctx.getNumInputs() == 4) {

Current shape inference code will pick sizes input only if number of inputs is 4 but with roi and scales being optional inputs shape inference will fail.

Signed-off-by: daquexian <daquexian566@gmail.com>

daquexian · 2020-10-14T02:25:47Z

@daquexian : The tests are checked in with the test script. So you need to update the generated tests as well. I think you only updated the test script.

@askhade Thanks! Updated.

Current shape inference code will pick sizes input only if number of inputs is 4 but with roi and scales being optional inputs shape inference will fail.

I don't think so.

onnx/onnx/shape_inference/implementation.h

Lines 59 to 65 in 88e5d3e

    
           for (const auto& input : n.input()) { 
        
             auto valueTypesIter = valueTypesByName.find(input); 
        
             if (valueTypesIter != valueTypesByName.end()) { 
        
               allInputTypes_.push_back(valueTypesIter->second); 
        
             } else { 
        
               allInputTypes_.push_back(nullptr); 
        
             }

onnx/onnx/shape_inference/implementation.h

Lines 87 to 89 in 88e5d3e

    
           size_t getNumInputs() const override { 
        
             return allInputTypes_.size(); 
        
           }

The allInputTypes_ will be [input_type, nullptr, nullptr, sizes_type] and the number of inputs will still be 4.

daquexian · 2020-10-14T02:47:29Z

It is true that the documentation-generator does not have infrastructure to fix the number of inputs expected. However, the type-and-shape-inference checker can be augmented to verify that exactly one of two inputs is specified. (Well, sort of ... we can do the check using both getNumInputs and getInputType; we will need to use getInputType as a proxy for whether a particular input is specified; it is not perfect, but it is reasonable. The only minor issue would be a misleading error-message in the case where an input is specified, but type-inference could not infer its type.

Good advice! Let's merge this PR for 1.8 release and I can submit another pr for this in the future.

daquexian requested a review from a team as a code owner September 20, 2020 06:05

daquexian added 2 commits September 20, 2020 14:05

make scales as an optional input, and remove tf_half_pixel_for_nn in …

23ecb75

…favor of half_pixel + round_prefer_ceil Signed-off-by: daquexian <daquexian566@gmail.com>

regenerate docs

f120eff

Signed-off-by: daquexian <daquexian566@gmail.com>

daquexian force-pushed the update_resize branch from f9db08e to f120eff Compare September 20, 2020 06:08

Merge branch 'master' into update_resize

90cb711

hariharans29 reviewed Oct 1, 2020

View reviewed changes

askhade reviewed Oct 2, 2020

View reviewed changes

askhade added this to the 1.8 milestone Oct 2, 2020

daquexian added 2 commits October 5, 2020 11:28

update defs.cc according to review

d67c978

Signed-off-by: daquexian <daquexian566@gmail.com>

Merge branch 'update_resize' of github.com:daquexian/onnx into update…

36a9af9

…_resize

hariharans29 reviewed Oct 7, 2020

View reviewed changes

hariharans29 approved these changes Oct 12, 2020

View reviewed changes

daquexian added 2 commits October 12, 2020 11:23

fix typo: size->sizes

a2a7be7

Signed-off-by: daquexian <daquexian566@gmail.com>

Merge remote-tracking branch 'origin/master' into update_resize

5538783

daquexian force-pushed the update_resize branch from bf1a53f to 5538783 Compare October 12, 2020 03:23

Merge branch 'master' into update_resize

9cbd08b

askhade self-requested a review October 12, 2020 19:02

askhade requested changes Oct 12, 2020

View reviewed changes

askhade added the operator Issues related to ONNX operators label Oct 12, 2020

daquexian added 3 commits October 13, 2020 16:02

update test for optional roi

b9f9094

Signed-off-by: daquexian <daquexian566@gmail.com>

Merge branch 'update_resize' of github.com:daquexian/onnx into update…

399f046

…_resize

Merge remote-tracking branch 'origin/master' into update_resize

c27d9ee

daquexian added 3 commits October 14, 2020 09:53

re-generate tests

4b7786a

Signed-off-by: daquexian <daquexian566@gmail.com>

Merge remote-tracking branch 'origin/master' into update_resize

fd82156

regenerate docs

d5f7ddf

Signed-off-by: daquexian <daquexian566@gmail.com>

gramalingam approved these changes Oct 14, 2020

View reviewed changes

askhade approved these changes Oct 14, 2020

View reviewed changes

askhade merged commit fbeb7b1 into onnx:master Oct 14, 2020

daquexian deleted the update_resize branch October 14, 2020 03:29

daquexian mentioned this pull request Jul 7, 2021

unable to use resize with sizes (must set roi)? #3500

Closed

askhade mentioned this pull request Jan 21, 2022

Resize tf_half_pixel_for_nn #3924

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

mark scales as optional and remove tf_half_pixel_for_nn in resize op #3026

mark scales as optional and remove tf_half_pixel_for_nn in resize op #3026

daquexian commented Sep 20, 2020 •

edited

hariharans29 Oct 1, 2020

daquexian Oct 5, 2020 •

edited

askhade Oct 2, 2020

askhade Oct 2, 2020

daquexian Oct 3, 2020

askhade commented Oct 2, 2020

daquexian commented Oct 5, 2020

hariharans29 commented Oct 7, 2020 •

edited

hariharans29 Oct 7, 2020

daquexian Oct 12, 2020

hariharans29 Oct 7, 2020

daquexian Oct 12, 2020

hariharans29 commented Oct 7, 2020

daquexian commented Oct 12, 2020

hariharans29 left a comment

askhade left a comment •

edited

daquexian commented Oct 13, 2020

askhade commented Oct 13, 2020

gramalingam commented Oct 13, 2020

gramalingam commented Oct 13, 2020

askhade commented Oct 13, 2020

daquexian commented Oct 14, 2020 •

edited

daquexian commented Oct 14, 2020

mark scales as optional and remove tf_half_pixel_for_nn in resize op #3026

mark scales as optional and remove tf_half_pixel_for_nn in resize op #3026

Conversation

daquexian commented Sep 20, 2020 • edited

Choose a reason for hiding this comment

daquexian Oct 5, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

askhade commented Oct 2, 2020

daquexian commented Oct 5, 2020

hariharans29 commented Oct 7, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hariharans29 commented Oct 7, 2020

daquexian commented Oct 12, 2020

hariharans29 left a comment

Choose a reason for hiding this comment

askhade left a comment • edited

Choose a reason for hiding this comment

daquexian commented Oct 13, 2020

askhade commented Oct 13, 2020

gramalingam commented Oct 13, 2020

gramalingam commented Oct 13, 2020

askhade commented Oct 13, 2020

daquexian commented Oct 14, 2020 • edited

daquexian commented Oct 14, 2020

daquexian commented Sep 20, 2020 •

edited

daquexian Oct 5, 2020 •

edited

hariharans29 commented Oct 7, 2020 •

edited

askhade left a comment •

edited

daquexian commented Oct 14, 2020 •

edited