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

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

Closed
wants to merge 11 commits into from

Conversation

@vandanavk
Copy link
Contributor

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

  • Fix for bilinear upsampling documentation and test are at #14035

@vandanavk vandanavk requested a review from szha as a code owner Feb 1, 2019

@vandanavk

This comment has been minimized.

Copy link
Contributor Author

commented Feb 1, 2019

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

@vandanavk vandanavk changed the title [WIP] Support tuple of scales in upsample operator Support tuple of scales in upsample operator Feb 1, 2019

@vandanavk vandanavk force-pushed the vandanavk:backend_for_onnx branch from fb8046f to f5dd911 Feb 1, 2019

@vandanavk

This comment has been minimized.

Copy link
Contributor Author

commented Feb 1, 2019

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

@vandanavk vandanavk changed the title Support tuple of scales in upsample operator [MXNET-891] Support tuple of scales in upsample operator Feb 1, 2019

@vandanavk vandanavk force-pushed the vandanavk:backend_for_onnx branch from f5dd911 to e97232d Feb 1, 2019

@ChaiBapchya
Copy link
Contributor

left a comment

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

}

// perform the upsampling
int i0, i1, i2, i3;

This comment has been minimized.

Copy link
@ChaiBapchya

ChaiBapchya Feb 5, 2019

Contributor

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

This comment has been minimized.

Copy link
@vandanavk

vandanavk Feb 15, 2019

Author Contributor

Sure, I'll make this change

@ankkhedia

This comment has been minimized.

Copy link
Contributor

commented Feb 14, 2019

@Roshrini Ping for review!

@vandanavk vandanavk force-pushed the vandanavk:backend_for_onnx branch from e97232d to b2a6219 Feb 14, 2019

@vandanavk

This comment has been minimized.

Copy link
Contributor Author

commented Feb 25, 2019

@samskalicky
Copy link
Contributor

left a comment

LGTM

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

for (i0 = 0; i0 < osz0; i0++) {

This comment has been minimized.

Copy link
@anirudhacharya

anirudhacharya Feb 25, 2019

Contributor

can this nested for loop be vectorized?

@vandanavk

This comment has been minimized.

Copy link
Contributor Author

commented Mar 7, 2019

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

@pinaraws

This comment has been minimized.

Copy link

commented Mar 20, 2019

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

@piyushghai

This comment has been minimized.

Copy link
Contributor

commented Apr 9, 2019

@vandanavk Gentle ping...

@Roshrini

This comment has been minimized.

Copy link
Member

commented Apr 17, 2019

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

@huangzhiyuan

This comment has been minimized.

Copy link
Contributor

commented Apr 23, 2019

@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?

@pengzhao-intel
Copy link
Contributor

left a comment

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

This comment has been minimized.

Copy link
Contributor Author

commented May 9, 2019

@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

This comment has been minimized.

Copy link
Contributor

commented May 21, 2019

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

@AnaRhisT94
Copy link

left a comment

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

This comment has been minimized.

Copy link
Contributor

commented Jul 4, 2019

@vandanavk

This comment has been minimized.

Copy link
Contributor Author

commented Jul 30, 2019

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

@vandanavk

This comment has been minimized.

Copy link
Contributor Author

commented Aug 8, 2019

Closing in favor of #15811.

@vandanavk vandanavk closed this Aug 8, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.