Fix upsample_bilinear to respect align_corner argument #1254
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #1254 +/- ##
==========================================
- Coverage 78.78% 78.77% -0.01%
==========================================
Files 119 119
Lines 15679 15691 +12
Branches 2481 2483 +2
==========================================
+ Hits 12353 12361 +8
- Misses 2919 2921 +2
- Partials 407 409 +2 ☔ View full report in Codecov by Sentry. |
|
@justinchuby The reason why it passes on onnxscript side is that we are not testing the We need more e2e pytorch export tests when adding ops in torchlib to ensure it works. |
BowenBao
added
the
topic: torch_lib
|
Thanks!
Do you have suggestions? Currently we rely on tests in PyTorch for integration tests. |
| result = _aten_upsample_bilinear2d_output_size(self, output_size) | ||
| result = _aten_upsample_bilinear2d_output_size( | ||
| self, output_size, coordinate_transformation_mode | ||
| ) | ||
| else: | ||
| assert scales_h is not None | ||
| assert scales_h == scales_w, f"scale_h({scales_h}) != scale_w({scales_w})" |
There was a problem hiding this comment.
Unclear to me, looks like different scale values are not covered in tests.
Test Results 24 files ±0 24 suites ±0 1h 35m 57s ⏱️ - 10m 57s For more details on these failures, see this check. Results for commit ab8ec04. ± Comparison against base commit 5b9c318. ♻️ This comment has been updated with latest results. |
Based on current infra, one would need to add both test cases to onnxscript and pytorch ... Otherwise, the wrangler itself and operator signature implementation part is missing coverage. |
…function torch.nn.functional.upsample_bilinear
| if output_size is not None: | ||
| result = _aten_upsample_bilinear2d_output_size(self, output_size) | ||
| result = _aten_upsample_bilinear2d_output_size( | ||
| self, output_size, coordinate_transformation_mode | ||
| ) | ||
| else: | ||
| assert scales_h is not None | ||
| assert scales_h == scales_w, f"scale_h({scales_h}) != scale_w({scales_w})" |
There was a problem hiding this comment.
Should we reverse the check - omit output_size when the scales are not None?
There was a problem hiding this comment.
Do you have link to spec whether which one takes precedence over the other?
There was a problem hiding this comment.
The reference here shows scale is considered first if present: https://github.com/pytorch/pytorch/blob/f6767244cf292cb5e434303fe62e576b456e92eb/torch/_decomp/decompositions.py#L3311-L3315
There was a problem hiding this comment.
But testing suggested otherwise
Co-authored-by: Justin Chu <justinchuby@users.noreply.github.com>
Fixes #1159 (comment) which indeed turns out to be a problem uncovered by PyTorch CI https://github.com/pytorch/pytorch/actions/runs/7508784822/job/20445196351?pr=117314.
Above is outdated. The case is more complicated. #1254 (comment).
In short this PR fixes the torchlib op signature to match with aten spec, and updates input wrangler for test case to bridge from sample test inputs for function
torch.nn.functional.upsample_bilinear.