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

Add Col2Im CPU op #12311

Merged
merged 35 commits into from
Jan 25, 2023
Merged

Add Col2Im CPU op #12311

merged 35 commits into from
Jan 25, 2023

Conversation

thiagocrepaldi
Copy link
Contributor

@thiagocrepaldi thiagocrepaldi commented Jul 25, 2022

Description
This PR implements N-dimensional Col2Im as a contrib CPU Op as specified by ONNX's onnx/onnx#3948

Motivation and Context

  • Col2Im enables models such as:
  • It also serves to document the ORT's obscure math::Col2ImNd utility

@lgtm-com
Copy link

lgtm-com bot commented Jul 26, 2022

This pull request introduces 1 alert when merging bedbfcd into c40f73a - view on LGTM.com

new alerts:

  • 1 for Unused local variable

@thiagocrepaldi thiagocrepaldi force-pushed the thiagofc/add-col2im-contrib-op branch 3 times, most recently from 21f94d9 to d6cc30b Compare July 26, 2022 20:01
@lgtm-com
Copy link

lgtm-com bot commented Jul 26, 2022

This pull request introduces 4 alerts when merging d6cc30b into 51a7998 - view on LGTM.com

new alerts:

  • 3 for Commented-out code
  • 1 for Unused local variable

@thiagocrepaldi thiagocrepaldi force-pushed the thiagofc/add-col2im-contrib-op branch from d6cc30b to c5b72eb Compare July 26, 2022 22:07
@lgtm-com
Copy link

lgtm-com bot commented Jul 27, 2022

This pull request introduces 4 alerts when merging c5b72eb into 51a7998 - view on LGTM.com

new alerts:

  • 3 for Commented-out code
  • 1 for Unused local variable

@thiagocrepaldi thiagocrepaldi force-pushed the thiagofc/add-col2im-contrib-op branch 2 times, most recently from a5fbfa0 to 4cfde71 Compare July 27, 2022 21:49
@lgtm-com
Copy link

lgtm-com bot commented Jul 27, 2022

This pull request introduces 1 alert when merging 4cfde71 into 148b1ef - view on LGTM.com

new alerts:

  • 1 for Unused local variable

@lgtm-com
Copy link

lgtm-com bot commented Aug 2, 2022

This pull request introduces 5 alerts when merging 71100b3 into 315e006 - view on LGTM.com

new alerts:

  • 4 for Commented-out code
  • 1 for Unused local variable

@thiagocrepaldi thiagocrepaldi force-pushed the thiagofc/add-col2im-contrib-op branch from 71100b3 to 72e056c Compare August 2, 2022 03:23
@lgtm-com
Copy link

lgtm-com bot commented Aug 2, 2022

This pull request introduces 4 alerts when merging 72e056c into 5d1173f - view on LGTM.com

new alerts:

  • 3 for Commented-out code
  • 1 for Unused local variable

@lgtm-com
Copy link

lgtm-com bot commented Aug 3, 2022

This pull request introduces 4 alerts when merging 346dea5 into d1497bd - view on LGTM.com

new alerts:

  • 3 for Commented-out code
  • 1 for Unused local variable

@thiagocrepaldi thiagocrepaldi force-pushed the thiagofc/add-col2im-contrib-op branch from 346dea5 to 4382687 Compare August 3, 2022 23:01
@lgtm-com
Copy link

lgtm-com bot commented Aug 4, 2022

This pull request introduces 4 alerts when merging 4382687 into 97268e0 - view on LGTM.com

new alerts:

  • 3 for Commented-out code
  • 1 for Unused local variable

@thiagocrepaldi thiagocrepaldi force-pushed the thiagofc/add-col2im-contrib-op branch 2 times, most recently from c189515 to 0841772 Compare August 4, 2022 23:18
@lgtm-com
Copy link

lgtm-com bot commented Aug 5, 2022

This pull request introduces 4 alerts when merging 0841772 into 37995a7 - view on LGTM.com

new alerts:

  • 3 for Commented-out code
  • 1 for Unused local variable

@thiagocrepaldi thiagocrepaldi force-pushed the thiagofc/add-col2im-contrib-op branch from 0841772 to 4e1cfbd Compare August 9, 2022 22:36
@lgtm-com
Copy link

lgtm-com bot commented Aug 9, 2022

This pull request introduces 1 alert when merging 4e1cfbd into 0d9a02e - view on LGTM.com

new alerts:

  • 1 for Unused local variable

@thiagocrepaldi thiagocrepaldi force-pushed the thiagofc/add-col2im-contrib-op branch 2 times, most recently from c2d3822 to 13e326b Compare August 10, 2022 22:43
@lgtm-com
Copy link

lgtm-com bot commented Aug 10, 2022

This pull request introduces 1 alert when merging 13e326b into 3e78f3c - view on LGTM.com

new alerts:

  • 1 for Unused local variable

@thiagocrepaldi thiagocrepaldi force-pushed the thiagofc/add-col2im-contrib-op branch from 13e326b to 21bc8f5 Compare August 10, 2022 23:36
Signed-off-by: Liqun Fu <[email protected]>
Copy link
Contributor

@pranavsharma pranavsharma left a comment

Choose a reason for hiding this comment

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

Can you add unit tests? Also, why does the title say contrib op?

Signed-off-by: Liqun Fu <[email protected]>
@yuslepukhin
Copy link
Member

Anubis run is necessary. It is easier to catch regression on a specific PR rather than hunt then down later.

Signed-off-by: Liqun Fu <[email protected]>
@liqunfu liqunfu changed the title Add Col2Im contrib CPU op Add Col2Im CPU op Jan 20, 2023
@liqunfu
Copy link
Contributor

liqunfu commented Jan 20, 2023

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.

Signed-off-by: Liqun Fu <[email protected]>
Signed-off-by: Liqun Fu <[email protected]>
@liqunfu
Copy link
Contributor

liqunfu commented Jan 23, 2023

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.

@liqunfu liqunfu closed this Jan 23, 2023
@liqunfu liqunfu reopened this Jan 23, 2023
@faxu faxu added the triage:approved Approved for cherrypicks for release label Jan 25, 2023
pranavsharma
pranavsharma previously approved these changes Jan 25, 2023
@liqunfu liqunfu merged commit 32c05fc into main Jan 25, 2023
@liqunfu liqunfu deleted the thiagofc/add-col2im-contrib-op branch January 25, 2023 20:23
rui-ren pushed a commit that referenced this pull request Jan 27, 2023
**Description**
This PR implements N-dimensional Col2Im as a contrib CPU Op as specified
by ONNX's onnx/onnx#3948

**Motivation and Context**
- Col2Im enables models such as:
  - [SS-DCNet](https://github.com/xhp-hust-2018-2011/SS-DCNet)
  - [DSTT](https://github.com/ruiliu-ai/DSTT)
- It also serves to document the ORT's obscure `math::Col2ImNd` utility

Signed-off-by: Liqun Fu <[email protected]>
Co-authored-by: Liqun Fu <[email protected]>
@faxu faxu removed the release:1.14 label Feb 1, 2023
@mszhanyi
Copy link
Contributor

mszhanyi commented Feb 15, 2023

@thiagocrepaldi @liqunfu
onnx/onnx#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

@thiagocrepaldi
Copy link
Contributor Author

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])

@thiagocrepaldi
Copy link
Contributor Author

thiagocrepaldi commented Feb 15, 2023

That is odd, on ONNX main branch, this is correct. 24 is there and was fixed by @liqun at onnx/onnx#4769

It is safe to ignore this failure

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
triage:approved Approved for cherrypicks for release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants