Conversation
The Upsample converter only handled the opset 7/8 form and was wrong or unusable in several ways: - Since opset 9 `scales` is an input, but the converter still read the attribute, so every Upsample-9 model failed with `len(None)`. - Nearest mode used resize2d's default `round` rounding, while Upsample maps source indices with floor, so the output values were wrong even for plain 2x upsampling. - Every input dimension was converted with `int()`, so models with a dynamic batch or spatial extent failed with `int(Var)`. - Only 4-D inputs were accepted, via bare asserts. Resize-10 takes `(X, scales)` like Upsample-9, but it was dispatched to the Resize-18 converter, which read `scales` as `roi` and failed for all Resize-10 models. Lower both through a shared helper that follows the legacy semantics: output extents are floor(input * scale), coordinates are asymmetric, and nearest rounds the source index down when upsampling and up when downsampling (Resize-10 allows scales below one). Symbolic extents with integer scales stay exact integer products so they unify with other shapes, and 3-D/5-D inputs are handled with resize1d/resize3d. Unsupported configurations (batch or channel scaling, or nearest mode mixing up- and downsampling axes) raise OpAttributeUnImplemented. Resize-11 to 17 keep using the existing converter.
Contributor
Author
|
Hi @tlopex, could you help review this when you have time? It fixes the legacy |
This branch has not been deployed
This file contains hidden or 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
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.
The ONNX importer's handling of the legacy upsampling ops was broken in ways that affect most real models that use them.
Upsampleonly exists in opsets 7 to 9 andResize-10is its direct successor, so these are exactly the ops found in older exported models such as YOLOv3-era detectors.For
Upsample-9,scalesbecame an input, but the converter still read the removed attribute, so every opset 9 model failed withTypeError: object of type 'NoneType' has no len(). ForUpsample-7/8the model imported, but nearest mode was lowered withresize2d's defaultroundrounding. Upsample maps each output index tofloor(x_out / scale), so the imported model silently produced wrong values even for plain 2x upsampling (max abs diff around 4.8 against onnxruntime on random input). Every input dimension was also forced throughint(), so a dynamic batch or spatial extent failed withint(Var), and non-4-D inputs were rejected by bare asserts.Resize-10has the signature(X, scales)and the same semantics asUpsample-9, but because only_impl_v18existed it was dispatched to the Resize-18 converter, which interpretedscalesasroiand failed for every Resize-10 model.This PR lowers
Upsample-7,Upsample-9andResize-10through one helper that follows the legacy semantics as implemented by onnxruntime. Output extents arefloor(input * scale), coordinates are asymmetric, and nearest mode rounds the source index down when upsampling and up when downsampling (Resize-10 accepts scales below one). Symbolic extents with integer scales are kept as exact products such as2 * H, so they still unify with other shapes, for example a skip connection concatenated after the upsample. 3-D and 5-D inputs go throughresize1dandresize3d. Scaling the batch or channel dimension, or mixing upsampling and downsampling axes in nearest mode, raisesOpAttributeUnImplementedinstead of an assertion.Resize-11throughResize-17are explicitly mapped to the existing converter so their behaviour does not change.The new tests compare against onnxruntime for all three op versions across nearest and linear modes, 3-D/4-D/5-D inputs, a non-integer scale, Resize-10 downsampling, and symbolic batch and spatial dimensions. 26 of the 28 fail without this change; the other two guard behaviour that was already correct.
Two related cases are not covered here. Linear mode with a non-integer scale where
input * scaleis not an integer still differs from onnxruntime, because TOPI derives the coordinate ratio from the output size rather than the given scale; that is the problem addressed for Resize by #19698. Linear mode with a symbolic extent and an integer scale hits a separate crash in the interval-set analysis, fixed in #20414.