[Relax][ONNX] Support Resize dynamic ROI via TOPI#18963
[Relax][ONNX] Support Resize dynamic ROI via TOPI#18963Dayuxiaoshui wants to merge 1 commit intoapache:mainfrom
Conversation
The ONNX Resize converter previously rejected non-constant ROI inputs, which blocked models where ROI is provided at runtime. This change adds a dynamic-ROI path lowered through TOPI resize kernels while preserving the existing relax.image.resize* path for static ROI. Specifically: - add reusable helper to convert ONNX full ROI ([starts..., ends...]) into spatial ROI vector - add reusable helper to emit topi.image.resize1d/2d/3d for dynamic ROI - keep static ROI fast path for relax.image.resize2d/resize3d - normalize dynamic ROI expr before emit_te to ensure struct_info is populated - handle optional Resize inputs (roi/scales/sizes) more defensively - add frontend test coverage with graph-input ROI: test_resize_dynamic_roi_tf_crop_and_resize Ref: apache#18945
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces support for dynamic Region of Interest (ROI) in the ONNX Resize operator by leveraging TOPI's dynamic-ROI path. It adds helper functions to map ONNX ROI to spatial ROI and emit TOPI resize operations for 1D, 2D, and 3D data. The review feedback identifies a bug in the ROI index mapping for 3D resizing, where the order must be adjusted to match TOPI's expectations, and points out an invalid assertion that always evaluates to true and should be replaced with a proper exception.
| def resize3d_dyn(d, r, s0, s1, s2): | ||
| return topi.image.resize3d( | ||
| d, | ||
| (r[0], r[1], r[2], r[3], r[4], r[5]), |
There was a problem hiding this comment.
The ROI indices for 3D resize are incorrectly mapped. TOPI's resize3d implementation (in topi/image/resize.py) expects the ROI vector in the order [start_w, start_h, start_d, end_w, end_h, end_d]. However, _onnx_resize_spatial_roi_vector produces [d1, h1, w1, d2, h2, w2] for rank 5. You need to reorder the indices when passing them to topi.image.resize3d.
| (r[0], r[1], r[2], r[3], r[4], r[5]), | |
| (r[2], r[1], r[0], r[5], r[4], r[3]), |
| @@ -2706,11 +2794,26 @@ def _impl_v18(cls, bb, inputs, attr, params): | |||
| else: | |||
| assert f"Type {type(sizes)} for size is currently unsupported." | |||
There was a problem hiding this comment.
This assertion is invalid because a non-empty string literal is always truthy in Python, meaning the assertion will never trigger even if the code path is reached. It should be changed to raise ValueError or assert False.
| assert f"Type {type(sizes)} for size is currently unsupported." | |
| raise ValueError(f"Type {type(sizes)} for size is currently unsupported.") |
tlopex
left a comment
tlopex
left a comment
There was a problem hiding this comment.
The current test only covers the 2D case. Please add at least a 3D dynamic ROI test as well
| def resize3d_dyn(d, r, s0, s1, s2): | ||
| return topi.image.resize3d( | ||
| d, | ||
| (r[0], r[1], r[2], r[3], r[4], r[5]), |
| @@ -2706,11 +2794,26 @@ def _impl_v18(cls, bb, inputs, attr, params): | |||
| else: | |||
| assert f"Type {type(sizes)} for size is currently unsupported." | |||
2 participants
The ONNX Resize converter previously rejected non-constant ROI inputs, which blocked models where ROI is provided at runtime. This change adds a dynamic-ROI path lowered through TOPI resize kernels while preserving the existing relax.image.resize* path for static ROI.
Specifically:
Ref: #18945