-
Notifications
You must be signed in to change notification settings - Fork 53
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
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. |
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this still valid?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unclear to me, looks like different scale values are not covered in tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we reverse the check - omit output_size when the scales are not None?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you have link to spec whether which one takes precedence over the other?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
.