Skip to content
This repository has been archived by the owner on Nov 17, 2023. It is now read-only.

[MXNET-891] Support tuple of scales in upsample operator #14042

Closed
wants to merge 11 commits into from

Conversation

vandanavk
Copy link
Contributor

@vandanavk vandanavk commented Feb 1, 2019

Description

This PR adds the option to specify different scales for different dimensions.
Ref: #12602

Fixes #12851

Checklist

Essentials

Please feel free to remove inapplicable items for your PR.

  • The PR title starts with [MXNET-$JIRA_ID], where $JIRA_ID refers to the relevant JIRA issue created (except PRs with tiny changes)
  • Changes are complete (i.e. I finished coding on this PR)
  • All changes have test coverage:
  • Unit tests are added for small changes to verify correctness (e.g. adding a new operator)
  • Nightly tests are added for complicated/long-running ones (e.g. changing distributed kvstore)
  • Build tests will be added for build configuration changes (e.g. adding a new build option with NCCL)
  • Code is well-documented:
  • For user-facing API changes, API doc string has been updated.
  • For new C++ functions in header files, their functionalities and arguments are documented.
  • For new examples, README.md is added to explain the what the example does, the source of the dataset, expected performance on test set and reference to the original paper if applicable
  • Check the API doc at http://mxnet-ci-doc.s3-accelerate.dualstack.amazonaws.com/PR-$PR_ID/$BUILD_ID/index.html
  • To the my best knowledge, examples are either not affected by this change, or have been fixed to be compatible with this change

Changes

  • Change scale to tuple
  • Add custom nearest neighbor implementation
  • Allow same scales for bilinear upsampling
  • Add ONNX import/export for Upsampling operator

Comments

@vandanavk vandanavk requested a review from szha as a code owner February 1, 2019 06:30
@vandanavk
Copy link
Contributor Author

@mxnet-label-bot add [pr-work-in-progress, ONNX, Operator]

@marcoabreu marcoabreu added ONNX Operator pr-work-in-progress PR is still work in progress labels Feb 1, 2019
@vandanavk vandanavk changed the title [WIP] Support tuple of scales in upsample operator Support tuple of scales in upsample operator Feb 1, 2019
@vandanavk
Copy link
Contributor Author

@mxnet-label-bot update [pr-awaiting-review, ONNX, Operator]

@marcoabreu marcoabreu added pr-awaiting-review PR is waiting for code review and removed pr-work-in-progress PR is still work in progress labels Feb 1, 2019
@vandanavk vandanavk changed the title Support tuple of scales in upsample operator [MXNET-891] Support tuple of scales in upsample operator Feb 1, 2019
Copy link
Contributor

@ChaiBapchya ChaiBapchya left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution! Looks like quite a few CI jobs have failed - have a look.

}

// perform the upsampling
int i0, i1, i2, i3;
Copy link
Contributor

Choose a reason for hiding this comment

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

how about using index_t as datatype for all of them
Since for large operator support it was found to be useful -#13418

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, I'll make this change

@ankkhedia
Copy link
Contributor

@Roshrini Ping for review!

@vandanavk
Copy link
Contributor Author

vandanavk commented Feb 25, 2019

@anirudh2290 @samskalicky @apeforest for review

Copy link
Contributor

@samskalicky samskalicky left a comment

Choose a reason for hiding this comment

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

LGTM

int iout[4]; // Output indices
int iin[4]; // Input indices

for (i0 = 0; i0 < osz0; i0++) {
Copy link
Member

Choose a reason for hiding this comment

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

can this nested for loop be vectorized?

@vandanavk
Copy link
Contributor Author

@mxnet-label-bot update [ONNX, Operator, pr-work-in-progress]

@marcoabreu marcoabreu added pr-work-in-progress PR is still work in progress and removed pr-awaiting-review PR is waiting for code review labels Mar 7, 2019
@pinaraws
Copy link

@vandanavk Thanks for your contribution! Could you please rebase and resolve conflicts?

@piyushghai
Copy link
Contributor

@vandanavk Gentle ping...

@Roshrini
Copy link
Member

@vandanavk Can you resolve conflicts and look into build failures?

@huangzhiyuan
Copy link
Contributor

@vandanavk, thanks for your contribution, and I'm interested in this feature. But I have a question about it, have you tested the effect of this fix on the performance of the original int scale in upsample operator? I have a simple test script, and the result are below:

sym = mx.symbol.UpSampling(data, scale=2, sample_type='nearest')
OP ms ms/call calls
Upsampling (origin) 99.873 0.9511714285714287 105
Upsampling (with this fix) 736.018 7.009695238095238 105

could you help check the performance drop after you rebase and resolve conflicts?

Copy link
Contributor

@pengzhao-intel pengzhao-intel left a comment

Choose a reason for hiding this comment

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

Significant performance difference from the new implementation on the CPU.
@vandanavk could you help to fix it? Feel free to let me know if anything I can help :)

@vandanavk
Copy link
Contributor Author

@huangzhiyuan thanks for the script and performance numbers. I haven't had a chance to look into the performance yet. Will work on it soon.

@karan6181
Copy link
Contributor

@vandanavk Did you get a chance to work on it ? Thanks!

Copy link

@AnaRhisT94 AnaRhisT94 left a comment

Choose a reason for hiding this comment

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

How can I use the UpSampling code here?
Just copying and pasting that code into _op_transalations.py doesn’t work. it spits the error the 'UpSampling' isn't found. It’s probably because there’s a ‘get_inputs’ function which I don’t know where is located. and also it uses ‘onnx.’ i guess I should import onnx in the same file?

@ThomasDelteil
Copy link
Contributor

@vandanavk
Copy link
Contributor Author

@benhe2011 is working on optimizing the code based on @huangzhiyuan's comments.

@vandanavk
Copy link
Contributor Author

Closing in favor of #15811.

@vandanavk vandanavk closed this Aug 8, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
ONNX Operator pr-work-in-progress PR is still work in progress
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Error while converting onnx model to Mxnet