onnxruntime
onnxruntime copied to clipboard
microsoft
[WIP] Add Col2Im contrib CPU op
Description: Describe your changes.
Motivation and Context
- Why is this change required? What problem does it solve?
- If it fixes an open issue, please link to the issue here.
This pull request introduces 1 alert when merging bedbfcddf4be7eceedb970c0304912dca968ce19 into c40f73ae0cb986fd5e57c54764643adffbe2e37c - view on LGTM.com
new alerts:
- 1 for Unused local variable
![lgtm-com[bot] avatar](https://avatars.githubusercontent.com/in/17324?v=4 "Published by a pub.dev verified publisher"){kind=small}
This pull request introduces 4 alerts when merging d6cc30bd1cd6cd2d127fbd9ab2c6dbc4f23cb838 into 51a799802a9bc4f21ffc5ebd2979ccfefb376f2e - view on LGTM.com
new alerts:
- 3 for Commented-out code
- 1 for Unused local variable
This pull request introduces 4 alerts when merging c5b72eb61f6ba925d0da1cb120fcfa8e3d1ebe1c into 51a799802a9bc4f21ffc5ebd2979ccfefb376f2e - view on LGTM.com
new alerts:
- 3 for Commented-out code
- 1 for Unused local variable
This pull request introduces 1 alert when merging 4cfde7139001a1b70678f4c5cc4f63adf3df7588 into 148b1efe5e698138f473bd500778e78d49ee31ca - view on LGTM.com
new alerts:
- 1 for Unused local variable
This pull request introduces 5 alerts when merging 71100b32b0e94a4aa7d3c93d5f8360a33a04ee00 into 315e00653211b81129b4a8718f74d35afd4a2aaf - view on LGTM.com
new alerts:
- 4 for Commented-out code
- 1 for Unused local variable
This pull request introduces 4 alerts when merging 72e056cee2ba0d95382ba63db85567e12b278bfa into 5d1173fe68d97612f61dd2a240fe871cae926cba - view on LGTM.com
new alerts:
- 3 for Commented-out code
- 1 for Unused local variable
This pull request introduces 4 alerts when merging 346dea540d6d052313704bbf35565bf0454108aa into d1497bdf62b16a929762cdc71012b38aa7dbcaf1 - view on LGTM.com
new alerts:
- 3 for Commented-out code
- 1 for Unused local variable
This pull request introduces 4 alerts when merging 43826877e002a575a9c370d47ca0e1cc9eb6a333 into 97268e023c7f1c3e573666483126a0b03a9c42ad - view on LGTM.com
new alerts:
- 3 for Commented-out code
- 1 for Unused local variable
This pull request introduces 4 alerts when merging 08417728f29d45c77e213454e9d041ecd63d9ace into 37995a7245895c1116e67a90d72c4c568dac8282 - view on LGTM.com
new alerts:
- 3 for Commented-out code
- 1 for Unused local variable
This pull request introduces 1 alert when merging 4e1cfbd1161958eb6582116e2e6ee5f8420eccda into 0d9a02e64775c58080641e00fb1139dc3c687dcd - view on LGTM.com
new alerts:
- 1 for Unused local variable
This pull request introduces 1 alert when merging 13e326b98fbfa90f92d705e5154f6edd82f08736 into 3e78f3cf1f458f56652c0e4444b34658bd11b3b6 - view on LGTM.com
new alerts:
- 1 for Unused local variable
This pull request introduces 1 alert when merging 21bc8f552e1192755630ac9c52d0225dc73c5764 into 3e78f3cf1f458f56652c0e4444b34658bd11b3b6 - view on LGTM.com
new alerts:
- 1 for Unused local variable
This pull request introduces 1 alert when merging a06e041c3ea6a61563202417962ac1b7630fed02 into 3e78f3cf1f458f56652c0e4444b34658bd11b3b6 - view on LGTM.com
new alerts:
- 1 for Unused local variable
This pull request introduces 4 alerts when merging b43ff26c4484fa4872cda55ef3320b970e996f4b into 24eab921bee4dbf9002942fbc514b2b2d9ee3a64 - view on LGTM.com
new alerts:
- 3 for Commented-out code
- 1 for Unused local variable
This pull request introduces 4 alerts when merging d8e4791857c9994cb28efa4c72ea8dc26dab04cf into 580f2294bc50556a8e70cbaa02d39c595442efed - view on LGTM.com
new alerts:
- 3 for Commented-out code
- 1 for Unused local variable
Hi @askhade, Could you help reviewing this when you have some time or referring someone? Thanks
Hi @askhade, Could you help reviewing this when you have some time or referring someone? Thanks
gentle ping
@edgchen1 Could you help me reviewing this one? If not your area, would you know who could review it? I have been trying for the past 2 weeks to find the owner of "new ops department" at ORT, but couldn't get any traction yet
Anubis run is necessary. It is easier to catch regression on a specific PR rather than hunt then down later.
Can you add unit tests? Also, why does the title say contrib op?
Test cases are added to ONNX. These tests are run in CI. I disabled col2im_pads tests because there is a minor typo which will fail one test. Other tests for col2im are still running. @pranavsharma , If there is a specific need other than ONNX test cases?
The kernel was added as contrib op first before ONNX 1.13 is available. I removed "contrib" from the title.
[Edit]: I just brought back the tests @thiagocrepaldi wrote.
Can you add unit tests? Also, why does the title say contrib op?
Good point! @thiagocrepaldi already had test in this PR. I somehow removed the tests. I just brought back the tests Thiago wrote.
@thiagocrepaldi @liqunfu https://github.com/onnx/onnx/pull/4769 has been merged, but test_col2im_pads_cpu is still failed. Is it expected?
2023-02-15T03:34:57.7065257Z ======================================================================
2023-02-15T03:34:57.7065861Z FAIL: test_col2im_pads_cpu (__main__.OnnxBackendNodeModelTest)
2023-02-15T03:34:57.7066498Z ----------------------------------------------------------------------
2023-02-15T03:34:57.7066827Z Traceback (most recent call last):
2023-02-15T03:34:57.7067409Z File "/opt/hostedtoolcache/Python/3.8.15/x64/lib/python3.8/site-packages/onnx/backend/test/runner/__init__.py", line 296, in device_test_func
2023-02-15T03:34:57.7067857Z return test_func(*args, device=device, **kwargs)
2023-02-15T03:34:57.7068415Z File "/opt/hostedtoolcache/Python/3.8.15/x64/lib/python3.8/site-packages/onnx/backend/test/runner/__init__.py", line 360, in run
2023-02-15T03:34:57.7068811Z self.assert_similar_outputs(
2023-02-15T03:34:57.7069135Z File "onnx_backend_test_series.py", line 56, in assert_similar_outputs
2023-02-15T03:34:57.7069480Z assert_similar_array(ref_outputs[i], outputs[i])
2023-02-15T03:34:57.7069822Z File "onnx_backend_test_series.py", line 48, in assert_similar_array
2023-02-15T03:34:57.7070792Z np.testing.assert_allclose(ref_output, output, rtol=rtol, atol=atol)
2023-02-15T03:34:57.7072368Z File "/opt/hostedtoolcache/Python/3.8.15/x64/lib/python3.8/site-packages/numpy/testing/_private/utils.py", line 1530, in assert_allclose
2023-02-15T03:34:57.7072891Z assert_array_compare(compare, actual, desired, err_msg=str(err_msg),
2023-02-15T03:34:57.7073524Z File "/opt/hostedtoolcache/Python/3.8.15/x64/lib/python3.8/site-packages/numpy/testing/_private/utils.py", line 844, in assert_array_compare
2023-02-15T03:34:57.7073935Z raise AssertionError(msg)
2023-02-15T03:34:57.7074192Z AssertionError:
2023-02-15T03:34:57.7074581Z Not equal to tolerance rtol=0.001, atol=1e-05
2023-02-15T03:34:57.7074755Z
2023-02-15T03:34:57.7074991Z Mismatched elements: 1 / 25 (4%)
2023-02-15T03:34:57.7075261Z Max absolute difference: 10.
2023-02-15T03:34:57.7075538Z Max relative difference: 0.41666666
2023-02-15T03:34:57.7075818Z x: array([[[[ 8., 21., 24., 27., 14.],
2023-02-15T03:34:57.7076097Z [ 38., 66., 69., 72., 54.],
2023-02-15T03:34:57.7076502Z [ 68., 111., 114., 117., 84.],...
2023-02-15T03:34:57.7076815Z y: array([[[[ 8., 21., 24., 27., 24.],
2023-02-15T03:34:57.7077087Z [ 38., 66., 69., 72., 54.],
2023-02-15T03:34:57.7077357Z [ 68., 111., 114., 117., 84.],...
2023-02-15T03:34:57.7077508Z
2023-02-15T03:34:57.7077887Z ----------------------------------------------------------------------
2023-02-15T03:34:57.7078196Z Ran 2492 tests in 1.538s
2023-02-15T03:34:57.7078333Z
https://dev.azure.com/onnxruntime/onnxruntime/_build/results?buildId=891519&view=logs&j=7536d2cd-87d4-54fe-4891-bfbbf2741d83&t=7536d2cd-87d4-54fe-4891-bfbbf2741d83
Oh no, there is a typo in the ONNX test! It should actually be 24, not 14!
import torch
col = torch.tensor([[[1.0, 6.0, 11.0, 16.0, 21.0, 26, 31, 36, 41, 46, 51, 56, 61, 66, 71], # (1, 5, 15)
[2.0, 7.0, 12.0, 17.0, 22.0, 27, 32, 37, 42, 47, 52, 57, 62, 67, 72],
[ 3.0, 8.0, 13.0, 18.0, 23.0, 28, 33, 38, 43, 48, 53, 58, 63, 68, 73],
[ 4.0, 9.0, 14.0, 19.0, 24.0, 29, 34, 39, 44, 49, 54, 59, 64, 69, 74],
[ 5.0, 10.0, 15.0, 20.0, 25.0, 30, 35, 40, 45, 50, 55, 60, 65, 70, 75]]])
kernel_size = (1, 5)
output_size = (5, 5)
padding = (0, 1)
# Col2Im results
col2im = torch.nn.Fold(kernel_size=kernel_size,
padding=padding,
output_size=output_size)
im = col2im(col)
print(f'Col2Im output:\n{im}\n\t with shape {im.shape}')
Results in
Col2Im output:
tensor([[[[ 8., 21., 24., 27., 24.],
[ 38., 66., 69., 72., 54.],
[ 68., 111., 114., 117., 84.],
[ 98., 156., 159., 162., 114.],
[128., 201., 204., 207., 144.]]]])
with shape torch.Size([1, 1, 5, 5])
That is odd, on ONNX main branch, this is correct. 24 is there and was fixed by @liqun at https://github.com/onnx/onnx/pull/4769
It is safe to ignore this failure