Interp: fix bilinear/bicubic coordinate scale for non-integer scale_factor (same as #3555) - #6837
Interp: fix bilinear/bicubic coordinate scale for non-integer scale_factor (same as #3555)#6837Acture wants to merge 4 commits into
Conversation
…actor When resizing by a scale factor (no explicit output size), the coordinate scale should be 1/scale_factor, not w/outw (input over the rounded output). Tencent#3555 / 76e32e9 fixed this for nearest, but bilinear and bicubic still used w/outw in linear_coeffs/cubic_coeffs. For integer scales the two are equal so it was invisible; for a non-integer scale (e.g. 1.5x on 7 -> outw=10, 7/10=0.7 vs 1/1.5=0.667) every sample is offset, matching neither PyTorch's default (recompute_scale_factor=False) nor ONNX Resize half_pixel. linear_coeffs/cubic_coeffs now take the coordinate scale as an argument; each call site passes 1/width_scale (or 1/height_scale) in the scale-factor path and w/outw in the explicit-output-size path, exactly like the nearest path already does. align_corner and explicit-size are unchanged; integer scales stay bit-exact. Applied to every backend (naive/arm/x86/loongarch/mips/riscv + bf16/fp16 paths) and vulkan. Verified end-to-end against PyTorch on the naive and ARM backends (bilinear/bicubic scale=1.5 max|diff| 0.28/0.36 -> 0); test_interp stays green. Fixes Tencent#6836
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5a9bc896d9
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| const float hs = (output_height || !size_expr.empty()) ? h / (float)outh : 1.f / height_scale; | ||
| const float ws = (output_width || !size_expr.empty()) ? w / (float)outw : 1.f / width_scale; |
There was a problem hiding this comment.
When dynamic_target_size == 1 and no size_expr is set, outw/outh come from bottom_blobs[1] (resize(x0, size(x1))), but both new ternaries take the 1.f / *_scale branch because output_width/output_height remain zero. With the default scales this makes a dynamic resize such as 15x16 -> 12x10 use coord_scale 1 instead of 15/12 and 16/10, so bilinear/bicubic sample only the upper-left region; the same condition was copied into the optimized backends and should include the dynamic target-size case.
Useful? React with 👍 / 👎.
…blob) The coordinate-scale condition only checked output_width/size_expr, so the dynamic_target_size case (resize x0 to the size of a reference blob, param 5) fell through to 1/scale even though its output size is explicit. Add dynamic_target_size to the condition across all backends and vulkan, so the scale-factor branch is taken only for a real scale factor. Also fixes the same latent case in the pre-existing nearest path. Addresses the P1 review note on Tencent#6837.
|
Good catch — fixed in 57f739a. The condition now also checks `dynamic_target_size`, so the scale-factor branch (`1/scale`) is only taken for an actual scale factor; the reference-blob / explicit-size cases use `w/outw`. This also covers the same latent case in the nearest path. @codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 57f739a6c6
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| const float ws = (output_width || has_size_expr) ? w / (float)outw : 1.f / width_scale; | ||
| const float hs = (output_height || has_size_expr) ? h / (float)outh : 1.f / height_scale; |
There was a problem hiding this comment.
When opt.use_bf16_storage routes through forward_bf16s, the helper is called with only !size_expr.empty() as has_size_expr, so a reference-blob resize (dynamic_target_size=1, no explicit output size/size_expr) reaches this new branch as false and uses 1.f / width_scale instead of w / outw. Before this change the bf16 bilinear/bicubic coeffs always used w/outw, so dynamic reference resizes on x86 bf16 now sample the wrong coordinates; the caller needs to pass/check dynamic_target_size the same way the fp32 x86 path does.
Useful? React with 👍 / 👎.
The bf16 helper (interp_forward_bf16s_sse / _avx512bf16) only received has_size_expr, so a reference-blob resize (dynamic_target_size=1, no explicit output size / size_expr) fell through to the 1/scale branch and sampled wrong coordinates on the x86 bf16 storage path. Pass and check dynamic_target_size the same way the fp32 x86 path does. Addresses the P2 review note on Tencent#6837.
The layer change in this PR makes the coordinate scale 1/scale_factor, which is what PyTorch uses when recompute_scale_factor is False (its default when a scale factor is given). But pass_ncnn/F_interpolate.cpp matches recompute_scale_factor with a wildcard and always emits params 1/2, so recompute=True collapses onto the same ncnn parameters and would now be computed with the wrong scale. recompute_scale_factor=True means "derive the scale from the resulting output size", which is exactly what the explicit-output-size path (params 3/4) already does. So route it there: two new patterns (with and without align_corners, since the matcher requires the parameter sets to correspond exactly) registered at priority 19 so they are tried before the existing wildcard passes at 20. The output size is taken from the output operand's recorded shape when available and falls back to floor(in * scale). The operand shape is preferred because pnnx stores scale_factor as float32 while torch computed the size in float64, so e.g. 0.7f is 0.699999988 and floor(10 * 0.7f) yields 6 where torch yields 7. When neither is resolvable — no shape, or a -1 symbolic dimension — match() declines and the existing wildcard pass handles the op as before. Also drops the 14 five-argument coefficient wrappers this PR had added. A repo-wide scan of every .h/.c/.cpp finds no five-argument call sites; the PR described them as keeping old callers, and that was wrong. src/layer/interp.cpp never needed one either. Known gap, not addressed here: nn.Upsample(recompute_scale_factor=True) still reaches params 1/2, because pass_level1/nn_Upsample.cpp does not record the flag. Verified: arm64 and an x86_64 cross-build both compile and pass tests/test_interp. The new pnnx pass compiles clean but has not been executed — building pnnx needs libtorch. tools/pnnx/tests/ncnn/test_F_interpolate.py already contains eight recompute_scale_factor=True cases and is the oracle for this; it needs a CI run.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6011821887
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| const float hs = (output_height || dynamic_target_size || !size_expr.empty()) ? h / (float)outh : 1.f / height_scale; | ||
| const float ws = (output_width || dynamic_target_size || !size_expr.empty()) ? w / (float)outw : 1.f / width_scale; | ||
| linear_coeffs(w, outw, ws, xofs, alpha, align_corner); |
There was a problem hiding this comment.
This scale-factor branch now uses the non-recompute coordinate scale, but pnnx's nn_Upsample lowering still drops recompute_scale_factor=True and emits params 1/2 from scale_factor with size=None. For an nn.Upsample(..., mode='bilinear', align_corners=False, recompute_scale_factor=True) where input_size * scale_factor is not integral, converted models now sample with 1/scale_factor instead of PyTorch's input_size/output_size; the module pass needs the same explicit-output-size lowering added for F.interpolate.
Useful? React with 👍 / 👎.
| const int s = input_shape[input_shape.size() - dims + i]; | ||
| if (s < 1) | ||
| return false; |
There was a problem hiding this comment.
When pnnx records dynamic spatial dims as -1, this return false makes the new recompute_scale_factor=True pass decline; the later generic F.interpolate pass still matches recompute_scale_factor=* and writes scale-factor params, which this commit now interprets as recompute=False. A dynamic F.interpolate(..., scale_factor=1.5, recompute_scale_factor=True) therefore silently changes sampling from PyTorch's input_size/output_size behavior; emit a size expression or block the fallback for unresolved recompute=True cases.
Useful? React with 👍 / 👎.
1 participant
What
When
Interpresizes by a scale factor (no explicit output size), the coordinate scale should be1/scale_factor, notw/outw(input over the rounded output). #3555 / 76e32e9 fixed this for nearest, but bilinear and bicubic still usedw/outwinlinear_coeffs/cubic_coeffs.For integer scales the two are equal, so it never showed up. For a non-integer scale (1.5x on 7 → outw=10,
7/10=0.7vs1/1.5=0.667) every sample is offset — matching neither PyTorch's default (recompute_scale_factor=False) nor ONNX Resizehalf_pixel. No warning during conversion.Fixes #6836.
Repro (pnnx, 3x7x7 fp32, vs PyTorch — before this PR)
Fix
linear_coeffs/cubic_coeffsnow take the coordinate scale as an argument (with a thin wrapper keeping old callers). Each call site passes1/width_scale/1/height_scalein the scale-factor path andw/outwin the explicit-size path — same as the nearest path already does.align_cornerand explicit-size are untouched, and integer scales stay bit-exact.Applied across every backend (naive / arm / x86 / loongarch / mips / riscv incl. bf16/fp16 paths) and vulkan.
Verification
0.28 / 0.36 → 0, controls stay 0.test_interppasses locally (its existing non-integer bilinear/bicubic cases compare each backend against naive, so they guard cross-backend consistency).