Skip to content

Add Col2Im operator#3948

Merged
gramalingam merged 10 commits intoonnx:mainfrom
thiagocrepaldi:thiagofc/add-col2im
Aug 29, 2022
Merged

Add Col2Im operator#3948
gramalingam merged 10 commits intoonnx:mainfrom
thiagocrepaldi:thiagofc/add-col2im

Conversation

@thiagocrepaldi
Copy link
Copy Markdown

@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
Copy Markdown
Author

thiagocrepaldi commented Jan 18, 2022

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

@askhade
Copy link
Copy Markdown
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
Comment thread onnx/defs/tensor/defs.cc Outdated
askhade
askhade reviewed Jan 18, 2022
View reviewed changes
Comment thread onnx/defs/tensor/defs.cc Outdated
@askhade
Copy link
Copy Markdown
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
Copy Markdown
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
Comment thread onnx/backend/test/data/real/test_bvlc_alexnet/data.json
askhade
askhade reviewed Jan 19, 2022
View reviewed changes
Comment thread onnx/defs/tensor/defs.cc Outdated
askhade
askhade reviewed Jan 19, 2022
View reviewed changes
Comment thread onnx/defs/tensor/defs.cc Outdated
askhade
askhade reviewed Jan 19, 2022
View reviewed changes
Comment thread onnx/defs/tensor/defs.cc Outdated
askhade
askhade reviewed Jan 19, 2022
View reviewed changes
Comment thread onnx/defs/tensor/defs.cc Outdated
askhade
askhade reviewed Jan 19, 2022
View reviewed changes
Comment thread onnx/defs/tensor/defs.cc Outdated
askhade
askhade reviewed Jan 19, 2022
View reviewed changes
Comment thread onnx/defs/tensor/defs.cc Outdated
@askhade
Copy link
Copy Markdown
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 thiagocrepaldi mentioned this pull request Aug 11, 2022
Merged
@thiagocrepaldi
Copy link
Copy Markdown
Author

thiagocrepaldi commented Aug 12, 2022

microsoft/onnxruntime#12311 is now ready and implements Col2Im as contrib op on ORT

@thiagocrepaldi thiagocrepaldi force-pushed the thiagofc/add-col2im branch 2 times, most recently from cabdaba to 4cc3592 Compare August 12, 2022 01:24
@thiagocrepaldi thiagocrepaldi mentioned this pull request Aug 12, 2022
Closed
gramalingam
gramalingam reviewed Aug 15, 2022
View reviewed changes
Comment thread onnx/defs/nn/defs.cc Outdated
gramalingam
gramalingam reviewed Aug 15, 2022
View reviewed changes
Comment thread onnx/defs/nn/defs.cc Outdated
gramalingam
gramalingam reviewed Aug 15, 2022
View reviewed changes
Comment thread onnx/defs/operator_sets.h Outdated
gramalingam
gramalingam reviewed Aug 15, 2022
View reviewed changes
Comment thread onnx/backend/test/case/node/col2im.py
gramalingam
gramalingam reviewed Aug 16, 2022
View reviewed changes
Comment thread onnx/defs/nn/defs.cc Outdated
Comment thread onnx/defs/nn/defs.cc Outdated
Comment thread onnx/defs/nn/defs.cc Outdated

std::vector<int64_t> pads = {};
if (getRepeatedAttribute(ctx, "pads", pads)) {
if ((pads.size() != 0) && (pads.size() != n_input_dims * 2)) {
Copy link
Copy Markdown
Contributor

@gramalingam gramalingam Aug 16, 2022

Choose a reason for hiding this comment

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

Suggest replacing these 3 lines with

if (pad.size() % 2)
   fail_shape_inference("Attribute pads must have an even size");
unifyDim (n_input_dims, pads.size() / 2);

Comment thread onnx/defs/nn/defs.cc Outdated
Comment thread onnx/defs/nn/defs.cc Outdated
Comment thread onnx/defs/nn/defs.cc Outdated
Comment thread onnx/defs/nn/defs.cc Outdated
gramalingam
gramalingam reviewed Aug 16, 2022
View reviewed changes
Comment thread onnx/defs/nn/defs.cc
gramalingam
gramalingam reviewed Aug 16, 2022
View reviewed changes
Comment thread onnx/defs/nn/defs.cc
gramalingam
gramalingam reviewed Aug 17, 2022
View reviewed changes
Comment thread onnx/defs/nn/defs.cc
gramalingam
gramalingam reviewed Aug 17, 2022
View reviewed changes
Comment thread onnx/test/shape_inference_test.py
@thiagocrepaldi
Copy link
Copy Markdown
Author

thiagocrepaldi commented Aug 18, 2022

@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
Copy Markdown
Author

thiagocrepaldi commented Aug 25, 2022

@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 <thiago.crepaldi@microsoft.com>
Fix spec
fe7bb11 
Signed-off-by: Thiago Crepaldi <thiago.crepaldi@microsoft.com>
Address comments
79e4667 
Signed-off-by: Thiago Crepaldi <thiago.crepaldi@microsoft.com>
Address comments
89f5652 
Signed-off-by: Thiago Crepaldi <thiago.crepaldi@microsoft.com>
Add support to N-dimnesional input
3341aba 
Signed-off-by: Thiago Crepaldi <thiago.crepaldi@microsoft.com>
Update to opset 17
6bb1e53 
Signed-off-by: Thiago Crepaldi <thiago.crepaldi@microsoft.com>
Add Col2Im unit test for 5 dim image
8c6349c 
Signed-off-by: Thiago Crepaldi <thiago.crepaldi@microsoft.com>
Fix style
f651b40 
Signed-off-by: Thiago Crepaldi <thiago.crepaldi@microsoft.com>
Address comments
fb195b1 
Signed-off-by: Thiago Crepaldi <thiago.crepaldi@microsoft.com>
Address comments
d8a3916 
Signed-off-by: Thiago Crepaldi <thiago.crepaldi@microsoft.com>
askhade
askhade reviewed Aug 25, 2022
View reviewed changes
Copy link
Copy Markdown
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.

.

@thiagocrepaldi thiagocrepaldi mentioned this pull request Sep 2, 2022
Closed
@wrchen-voxel wrchen-voxel mentioned this pull request Oct 31, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@gramalingam gramalingam gramalingam approved these changes

+1 more reviewer

@askhade askhade askhade left review comments

Reviewers whose approvals may not affect merge requirements

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