Skip to content

Add Resize cubic mode without antialias (scales = [1, ≥1, ≥1, 1])#24385

Merged
yihonglyu merged 17 commits intomainfrom
yilyu/resize-cubic-no-antialias-between-scales-upscaling
Apr 15, 2025
Merged

Add Resize cubic mode without antialias (scales = [1, ≥1, ≥1, 1])#24385
yihonglyu merged 17 commits intomainfrom
yilyu/resize-cubic-no-antialias-between-scales-upscaling

Conversation

@yihonglyu
Copy link
Contributor

@yihonglyu yihonglyu commented Apr 10, 2025

Description

This PR adds support for the Resize operator in cubic mode without antialiasing (antialias=0). It supports scaling constraints of the form [1, scale_h, scale_w, 1], where scale_h ≥ 1 and scale_w ≥ 1.

Motivation and Context

The ARM64 Conv supports FP16, and we have an NhwcTransformer that transforms FP16 Conv to FP16 NhwcFusedConv. As a result, the subsequent Resize op also uses the NHWC format.

It support scales [1, x, y, 1] (x >= 1, y >= 1)
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.

Comments suppressed due to low confidence (3)

onnxruntime/test/providers/cpu/tensor/resize_op_test.cc:2292

  • The PR description notes support only for NHWC layout, yet this test is for NCHW. Please confirm if support for NCHW is intentional and update documentation or tests accordingly.
TEST(ResizeOpTest, NoAntialias_AlignCorners_Cubic_Floor_NCHW) {

onnxruntime/core/providers/cpu/tensor/upsamplebase.h:453

  • [nitpick] Consider simplifying or reformatting the error message to improve clarity when unsupported scale configurations are detected.
ORT_RETURN_IF_NOT(is_supported,

onnxruntime/core/providers/cpu/tensor/upsample.cc:1336

  • [nitpick] Consider renaming or adding an inline comment for 'upscaling' to clarify that it evaluates as true when at least one scale factor is ≥ 1.0f, as this behavior might be non-intuitive.
const bool upscaling = height_scale >= 1.0f || width_scale >= 1.0f;

Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

You can commit the suggested changes from lintrunner.

liqunfu
liqunfu previously approved these changes Apr 10, 2025
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
@yihonglyu yihonglyu changed the title Add Resize cubic mode without antialias (NHWC, scales = [1, ≥1, ≥1, 1]) Add Resize cubic mode without antialias (scales = [1, ≥1, ≥1, 1]) Apr 11, 2025
liqunfu
liqunfu previously approved these changes Apr 11, 2025
wejoncy
wejoncy previously approved these changes Apr 12, 2025
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

You can commit the suggested changes from lintrunner.

yihonglyu and others added 2 commits April 11, 2025 23:02
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
@yihonglyu yihonglyu requested a review from liqunfu April 12, 2025 14:45
@yihonglyu yihonglyu requested review from Copilot and wejoncy April 12, 2025 14:45
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.

Comments suppressed due to low confidence (2)

onnxruntime/core/providers/cpu/tensor/upsamplebase.h:447

  • [nitpick] Consider renaming 'outer_scale_inner_scale_one' to a more descriptive name (e.g., 'outer_and_innermost_scale_one') to improve readability.
const bool outer_scale_inner_scale_one = (scales[0] == 1.0f && scales[3] == 1.0f);

onnxruntime/core/providers/cpu/tensor/upsample.cc:1338

  • [nitpick] Consider adding a clarifying comment explaining why the anti-alias code path is executed when antialias is false but the scaling factors indicate upscaling; this will help future maintainers understand the design decision.
if (antialias_ || (!antialias_ && upscaling)) {

@yihonglyu yihonglyu requested a review from Copilot April 13, 2025 06:09
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.

Comments suppressed due to low confidence (2)

onnxruntime/test/providers/cpu/tensor/resize_op_test.cc:2292

  • For NCHW cubic mode without antialiasing, the expected scales vector should follow the pattern [1, ≥1, ≥1, 1] (with the first and last elements equal to 1). The provided scales {1, 2, 3, 4} violates this constraint since the last element is 4.
TEST(ResizeOpTest, NoAntialias_AlignCorners_Cubic_Floor_NCHW) {

onnxruntime/core/providers/cpu/tensor/upsamplebase.h:453

  • [nitpick] Consider improving the formatting of the error message string for clarity; the concatenation of string literals may lead to missing spaces between phrases.
ORT_RETURN_IF_NOT(is_supported,

@yihonglyu yihonglyu requested a review from Copilot April 14, 2025 18:39
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.

Comments suppressed due to low confidence (3)

onnxruntime/core/providers/cpu/tensor/upsamplebase.h:446

  • [nitpick] Consider renaming 'outer_scales_one' to 'outermost_scales_one' for increased clarity regarding which dimensions are expected to be 1.
const bool outer_scales_one = (scales[0] == 1.0f && scales[1] == 1.0f);

onnxruntime/core/providers/cpu/tensor/upsamplebase.h:453

  • [nitpick] The error message for unsupported scale values is quite verbose; consider reformatting or simplifying it to improve readability.
ORT_RETURN_IF_NOT(is_supported,

onnxruntime/test/providers/cpu/tensor/resize_op_test.cc:2393

  • [nitpick] Consider linking this FIXME to an issue tracker item or adding more context so that the kCudaExecutionProvider error can be properly tracked and addressed.
      // FIXME: Fix error on kCudaExecutionProvider

@yihonglyu yihonglyu requested a review from Copilot April 14, 2025 18:56
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.

Comments suppressed due to low confidence (2)

onnxruntime/test/providers/cpu/tensor/resize_op_test.cc:2393

  • The FIXME comment indicates an outstanding issue with kCudaExecutionProvider in the NHWC test. Consider adding a dedicated test case and tracking the issue externally to ensure complete coverage once the fix is in place.
      // FIXME: Fix error on kCudaExecutionProvider

onnxruntime/core/providers/cpu/tensor/upsample.cc:1340

  • [nitpick] The combined condition for invoking the antialias branch in non-NCHW upscaling scenarios might inadvertently apply antialiasing logic when the antialias flag is false. Please verify that this behavior is intended, and consider separating the conditions if different processing is needed for non-antialias upscaling cases.
if (antialias_ || (!is_nchw && upscaling)) {

@yihonglyu yihonglyu requested a review from Copilot April 14, 2025 19:16
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.

Comments suppressed due to low confidence (2)

onnxruntime/core/providers/cpu/tensor/upsample.cc:1336

  • [nitpick] Consider renaming 'upscaling' to 'is_upsampling' to more clearly convey that this flag determines when to follow the NHWC upsampling branch.
const bool upscaling = height_scale >= 1.0f && width_scale >= 1.0f;

onnxruntime/core/providers/cpu/tensor/upsample.cc:1352

  • Double-check that reusing the antialiasing logic in the NHWC branch (via NhwcResizeBiCubicAntiAlias) is intended for all upsampling cases when antialias is disabled, ensuring consistency with NCHW behavior.
      } else if (!is_nchw && upscaling) {

@yihonglyu yihonglyu requested a review from Copilot April 14, 2025 19:27
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.

Comments suppressed due to low confidence (1)

onnxruntime/test/providers/cpu/tensor/resize_op_test.cc:2292

  • The test for NCHW mode uses scales {1, 2, 3, 4} which does not match the documented pattern [1, scale_h, scale_w, 1] (expected last element to be 1). Please verify whether the test inputs or the scaling check logic in UpsampleBase should be updated for consistency.
TEST(ResizeOpTest, NoAntialias_AlignCorners_Cubic_Floor_NCHW) {

@yihonglyu yihonglyu merged commit 1f14dac into main Apr 15, 2025
85 of 89 checks passed
@yihonglyu yihonglyu deleted the yilyu/resize-cubic-no-antialias-between-scales-upscaling branch April 15, 2025 01:38
ashrit-ms pushed a commit that referenced this pull request Apr 24, 2025
…4385)

### Description
<!-- Describe your changes. -->

This PR adds support for the Resize operator in cubic mode without
antialiasing (antialias=0). It supports scaling constraints of the form
[1, scale_h, scale_w, 1], where scale_h ≥ 1 and scale_w ≥ 1.

### Motivation and Context
<!-- - Why is this change required? What problem does it solve?
- If it fixes an open issue, please link to the issue here. -->

The ARM64 Conv supports FP16, and we have an NhwcTransformer that
transforms FP16 Conv to FP16 NhwcFusedConv. As a result, the subsequent
Resize op also uses the NHWC format.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants