Skip to content
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

AddOp(upsample_bicubic2d) | feat(torchlib) #1208

Merged
merged 21 commits into from
Jan 3, 2024
Merged

Conversation

xiaowuhu
Copy link
Contributor

@xiaowuhu xiaowuhu commented Dec 7, 2023

No description provided.

@xiaowuhu xiaowuhu marked this pull request as draft December 7, 2023 10:32
Copy link

codecov bot commented Dec 7, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (1fa1ed6) 78.72% compared to head (8222049) 78.79%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1208      +/-   ##
==========================================
+ Coverage   78.72%   78.79%   +0.06%     
==========================================
  Files         118      118              
  Lines       15602    15645      +43     
  Branches     2468     2476       +8     
==========================================
+ Hits        12283    12327      +44     
  Misses       2911     2911              
+ Partials      408      407       -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link

github-actions bot commented Dec 7, 2023

Test Results

     24 files  ±     0      24 suites  ±0   1h 31m 58s ⏱️ - 5m 6s
 11 383 tests +     5   8 436 ✅ +    6    2 945 💤 ±     0    2 ❌  - 1 
257 610 runs   - 16 699  58 482 ✅  - 4 114  199 008 💤  - 12 584  120 ❌  - 1 

For more details on these failures, see this check.

Results for commit 8222049. ± Comparison against base commit 1fa1ed6.

This pull request removes 21 and adds 26 tests. Note that renamed tests count towards both.
onnxscript.tests.function_libs.torch_lib.ops_test.TestFunctionValidity ‑ test_function_has_op_schema_327_aten_upsample_nearest2d
onnxscript.tests.function_libs.torch_lib.ops_test.TestFunctionValidity ‑ test_function_has_op_schema_328_aten_ones_like
onnxscript.tests.function_libs.torch_lib.ops_test.TestFunctionValidity ‑ test_function_has_op_schema_329_aten_roll
onnxscript.tests.function_libs.torch_lib.ops_test.TestFunctionValidity ‑ test_function_has_op_schema_330_aten_roll_complex
onnxscript.tests.function_libs.torch_lib.ops_test.TestFunctionValidity ‑ test_function_has_op_schema_331_aten_scatter_reduce
onnxscript.tests.function_libs.torch_lib.ops_test.TestFunctionValidity ‑ test_function_has_op_schema_332_aten_slice_scatter
onnxscript.tests.function_libs.torch_lib.ops_test.TestFunctionValidity ‑ test_function_has_op_schema_333_aten_slice
onnxscript.tests.function_libs.torch_lib.ops_test.TestFunctionValidity ‑ test_function_has_op_schema_334_aten_stft
onnxscript.tests.function_libs.torch_lib.ops_test.TestFunctionValidity ‑ test_function_has_op_schema_335_aten_sum_dim_IntList
onnxscript.tests.function_libs.torch_lib.ops_test.TestFunctionValidity ‑ test_function_has_op_schema_336_aten_tensor_bool
…
onnxscript.function_libs.tools.torch_lib.deduce_type_constraints_test.TestDeduceTypeConstraints ‑ test_deduce_type_constraints_does_not_crash_for_onnx_function__aten_upsample_output_size
onnxscript.function_libs.tools.torch_lib.deduce_type_constraints_test.TestDeduceTypeConstraints ‑ test_deduce_type_constraints_does_not_crash_for_onnx_function__aten_upsample_scales
onnxscript.tests.function_libs.torch_lib.ops_test.TestFunctionValidity ‑ test_function_has_op_schema_327_aten_upsample_bicubic2d
onnxscript.tests.function_libs.torch_lib.ops_test.TestFunctionValidity ‑ test_function_has_op_schema_328_aten_upsample_nearest2d
onnxscript.tests.function_libs.torch_lib.ops_test.TestFunctionValidity ‑ test_function_has_op_schema_329_aten_ones_like
onnxscript.tests.function_libs.torch_lib.ops_test.TestFunctionValidity ‑ test_function_has_op_schema_330_aten_roll
onnxscript.tests.function_libs.torch_lib.ops_test.TestFunctionValidity ‑ test_function_has_op_schema_331_aten_roll_complex
onnxscript.tests.function_libs.torch_lib.ops_test.TestFunctionValidity ‑ test_function_has_op_schema_332_aten_scatter_reduce
onnxscript.tests.function_libs.torch_lib.ops_test.TestFunctionValidity ‑ test_function_has_op_schema_333_aten_slice_scatter
onnxscript.tests.function_libs.torch_lib.ops_test.TestFunctionValidity ‑ test_function_has_op_schema_334_aten_slice
…

♻️ This comment has been updated with latest results.

@xiaowuhu xiaowuhu marked this pull request as ready for review December 8, 2023 09:24
@xiaowuhu xiaowuhu marked this pull request as draft December 8, 2023 09:52
@xiaowuhu xiaowuhu marked this pull request as ready for review December 21, 2023 10:59
@justinchuby justinchuby changed the title AddOp(upstream_bicubic2d) | feat(torchlib) AddOp(upsample_bicubic2d) | feat(torchlib) Dec 21, 2023
Copy link
Collaborator

@justinchuby justinchuby left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you! Minor comments

onnxscript/function_libs/torch_lib/ops/nn.py Outdated Show resolved Hide resolved
Comment on lines 2207 to 2208
"""upsample_bicubic2d.vec(Tensor input, SymInt[]? output_size, bool align_corners, float[]? scale_factors) -> Tensor"""
"""upsample_bicubic2d(Tensor self, SymInt[2] output_size, bool align_corners, float? scales_h=None, float? scales_w=None) -> Tensor""" # pylint: disable=pointless-string-statement
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we split them into two functions?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Onnx.Resize() only accept either output_size or scales, it is error if both provided.
So for the second signature, when the output_size have been provided, the scales_h and scales_w will be ignored

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we handle when scales_h and scales_w are not None for upsample_bicubic2d?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Didn't handled.
Because from this signature upsample_bicubic2d(Tensor self, SymInt[2] output_size, bool align_corners, float? scales_h=None, float? scales_w=None) -> Tensor, the output_size must be provided, then we will ignore scales_h and scales_w.

Copy link
Collaborator

@justinchuby justinchuby left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you, and Happy New Year!!

@xiaowuhu xiaowuhu merged commit 1231cc0 into main Jan 3, 2024
33 of 37 checks passed
@xiaowuhu xiaowuhu deleted the xiaowu/addOp(upsample) branch January 3, 2024 05:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

Successfully merging this pull request may close these issues.

2 participants