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.

Sign up for GitHub

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

Jump to bottom

Add Col2Im operator #3948

Merged
merged 10 commits into from
Aug 29, 2022
Merged

Add Col2Im operator #3948

merged 10 commits into from
Aug 29, 2022

Conversation

thiagocrepaldi
Copy link

@thiagocrepaldi thiagocrepaldi commented Jan 14, 2022 •
edited
Loading

Description
Introduces an operator Col2Im() that rearranges input tensor in blocks. Same behavior as https://pytorch.org/cppdocs/api/function_namespaceat_1a979fbf85d8c7362d60d766bbf1639f10.html

Fixes #4106

Motivation and Context

microsoft/onnxruntime#12311 ORT implemented the spec proposed here for Col2Im with n-dimensional support as a contrib op.

@thiagocrepaldi thiagocrepaldi force-pushed the thiagofc/add-col2im branch 3 times, most recently from 628d371 to 4e5a34d Compare January 18, 2022 16:24
@thiagocrepaldi
Copy link
Author

@askhade Could you check whether I am going towards the right direction with this PR?

@askhade
Copy link
Contributor

askhade commented Jan 18, 2022

DCO is not signed for this PR. If you only have a single commit then it is easy to fix it... Just follow the directions here:
https://github.com/onnx/onnx/pull/3948/checks?check_run_id=4856175809

In future adding -s option to git commit adds sign off for that commit. example:
git commit -s -m "some comment"

askhade
askhade reviewed Jan 18, 2022
View reviewed changes
onnx/defs/tensor/defs.cc Outdated Show resolved Hide resolved
askhade
askhade reviewed Jan 18, 2022
View reviewed changes
onnx/defs/tensor/defs.cc Outdated Show resolved Hide resolved
@askhade
Copy link
Contributor

askhade commented Jan 18, 2022 •
edited
Loading

Few more comments:

  1. Run flake 8 in local repo. CIs are reporting errors from flake8
python -m pip install -q flake8
flake8 onnx
  1. Generate test data and add it to "https://github.com/onnx/onnx/tree/main/onnx/backend/test/data/node". This step picks your node unit test and creates onnx models + test data.
python onnx/backend/test/cmd_tools.py generate-data --op_type Col2Im --output <optional. provide output directory>

The script can be found here: https://github.com/onnx/onnx/blob/main/onnx/backend/test/cmd_tools.py#L90

  1. Make sure you are not manually editing the Changlelog.md, Operators.md and TestCoverage.md files. You can use this script to auto generate them:
python onnx/backend/test/stat_coverage.py
python onnx/defs/gen_doc.py
ONNX_ML=0 python onnx/defs/gen_doc.py

askhade
askhade previously requested changes Jan 18, 2022
View reviewed changes
Copy link
Contributor

@askhade askhade left a comment

Choose a reason for hiding this comment

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

added comments

@askhade askhade added topic: enhancement Request for new feature or operator topic: operator Issues related to ONNX operators labels Jan 18, 2022
@askhade askhade added this to the 1.11 milestone Jan 18, 2022
@thiagocrepaldi thiagocrepaldi force-pushed the thiagofc/add-col2im branch 4 times, most recently from 1498202 to 462b7fd Compare January 18, 2022 22:42
askhade
askhade reviewed Jan 19, 2022
View reviewed changes
onnx/defs/tensor/defs.cc Outdated Show resolved Hide resolved
askhade
askhade reviewed Jan 19, 2022
View reviewed changes
onnx/defs/tensor/defs.cc Outdated Show resolved Hide resolved
askhade
askhade reviewed Jan 19, 2022
View reviewed changes
onnx/defs/tensor/defs.cc Outdated Show resolved Hide resolved
askhade
askhade reviewed Jan 19, 2022
View reviewed changes
onnx/defs/tensor/defs.cc Outdated Show resolved Hide resolved
askhade
askhade reviewed Jan 19, 2022
View reviewed changes
onnx/defs/tensor/defs.cc Outdated Show resolved Hide resolved
askhade
askhade reviewed Jan 19, 2022
View reviewed changes
onnx/defs/tensor/defs.cc Outdated Show resolved Hide resolved
@askhade
Copy link
Contributor

askhade commented Jan 19, 2022

fix DCO

@thiagocrepaldi thiagocrepaldi force-pushed the thiagofc/add-col2im branch 3 times, most recently from d7644bc to b0878cb Compare January 19, 2022 19:42
@thiagocrepaldi
Copy link
Author

@askhade Could you dismiss your review? All comments were addressed back in the day, but I have no permission to dismiss nor request another review. Feel free to give it another go on the review too

@thiagocrepaldi
Copy link
Author

@askhade Could you dismiss your review? All comments were addressed back in the day, but I have no permission to dismiss nor request another review. Feel free to give it another go on the review too

gentle ping

Thiago Crepaldi added 10 commits August 25, 2022 14:24
Add Col2Im op to ONNX spec
58f345a 
Signed-off-by: Thiago Crepaldi <[email protected]>
Fix spec
fe7bb11 
Signed-off-by: Thiago Crepaldi <[email protected]>
Address comments
79e4667 
Signed-off-by: Thiago Crepaldi <[email protected]>
Address comments
89f5652 
Signed-off-by: Thiago Crepaldi <[email protected]>
Add support to N-dimnesional input
3341aba 
Signed-off-by: Thiago Crepaldi <[email protected]>
Update to opset 17
6bb1e53 
Signed-off-by: Thiago Crepaldi <[email protected]>
Add Col2Im unit test for 5 dim image
8c6349c 
Signed-off-by: Thiago Crepaldi <[email protected]>
Fix style
f651b40 
Signed-off-by: Thiago Crepaldi <[email protected]>
Address comments
fb195b1 
Signed-off-by: Thiago Crepaldi <[email protected]>
Address comments
d8a3916 
Signed-off-by: Thiago Crepaldi <[email protected]>
askhade
askhade reviewed Aug 25, 2022
View reviewed changes
Copy link
Contributor

@askhade askhade left a comment •
edited
Loading

Choose a reason for hiding this comment

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

.

@askhade askhade dismissed their stale review August 25, 2022 18:34

Taking request changes back

@gramalingam gramalingam merged commit 68859d3 into onnx:main Aug 29, 2022
@thiagocrepaldi thiagocrepaldi mentioned this pull request Sep 2, 2022
Closed
@thiagocrepaldi thiagocrepaldi deleted the thiagofc/add-col2im branch September 20, 2022 14:48
@wrchen-voxel wrchen-voxel mentioned this pull request Oct 31, 2022
Closed
liqunfu added a commit to microsoft/onnxruntime that referenced this pull request Jan 25, 2023
@liqunfu
Add Col2Im CPU op (#12311)
32c05fc 
**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]>
rui-ren pushed a commit to microsoft/onnxruntime that referenced this pull request Jan 27, 2023
@liqunfu @rui-ren
Add Col2Im CPU op (#12311)
30802e8 
**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]>
broune pushed a commit to broune/onnx that referenced this pull request May 6, 2023
Add Col2Im operator (onnx#3948)
cfce95b 
* Add Col2Im op to ONNX spec

Signed-off-by: Thiago Crepaldi <[email protected]>

* Fix spec

Signed-off-by: Thiago Crepaldi <[email protected]>

* Address comments

Signed-off-by: Thiago Crepaldi <[email protected]>

* Address comments

Signed-off-by: Thiago Crepaldi <[email protected]>

* Add support to N-dimnesional input

Signed-off-by: Thiago Crepaldi <[email protected]>

* Update to opset 17

Signed-off-by: Thiago Crepaldi <[email protected]>

* Add Col2Im unit test for 5 dim image

Signed-off-by: Thiago Crepaldi <[email protected]>

* Fix style

Signed-off-by: Thiago Crepaldi <[email protected]>

* Address comments

Signed-off-by: Thiago Crepaldi <[email protected]>

* Address comments

Signed-off-by: Thiago Crepaldi <[email protected]>

Signed-off-by: Thiago Crepaldi <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@askhade askhade askhade left review comments

@gramalingam gramalingam gramalingam approved these changes

Assignees
No one assigned
Labels
topic: enhancement Request for new feature or operator topic: operator Issues related to ONNX operators
Projects
PR Tracker
Status: Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add col2im operator
5 participants
@thiagocrepaldi @askhade @CLAassistant @garymm @gramalingam