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

Fix documentation for bilinear upsampling and add unit test #14035

Merged
merged 6 commits into from
Apr 17, 2019
Merged

Fix documentation for bilinear upsampling and add unit test #14035

merged 6 commits into from
Apr 17, 2019

Conversation

vandanavk
Copy link
Contributor

@vandanavk vandanavk commented Jan 31, 2019 •
edited
Loading

Description

Fixes #13078

Future work/alternate approaches:

  1. Initialize weight in the backend, so that the user doesn't have to give weights as input.
  2. Change backend to call contrib.BilinearResize2D

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

  • Additional documentation
  • test

Comments

Ref:
#9138
https://stackoverflow.com/questions/47897924/implementing-bilinear-interpolation-with-mxnet-ndarray-upsampling
#12970
#12983
#1412
https://discuss.mxnet.io/t/use-mx-nd-upsampling-with-bilinear-sample-type/2067/2

@vandanavk vandanavk requested a review from szha as a code owner January 31, 2019 08:17
@vandanavk
Copy link
Contributor Author

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

@marcoabreu marcoabreu added Operator pr-work-in-progress PR is still work in progress labels Feb 1, 2019
@vandanavk vandanavk mentioned this pull request Feb 1, 2019
Closed
5 tasks
@lupesko
Copy link
Contributor

lupesko commented Feb 18, 2019

@apeforest @anirudh2290 can you please help review?

anirudh2290
anirudh2290 reviewed Feb 18, 2019
View reviewed changes
src/operator/nn/upsampling-inl.h Outdated
.set_default(0);
DMLC_DECLARE_FIELD(sample_type)
.add_enum("nearest", up_enum::kNearest)
.add_enum("bilinear", up_enum::kBilinear)
.describe("upsampling method");
DMLC_DECLARE_FIELD(num_args).set_default(1)
Copy link
Member

Choose a reason for hiding this comment

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

why are we changing set_lower_bound to set_default. This is for proper error messaging when user provides bad inputs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

missed changing this back. will do that

python/mxnet/base.py Outdated
@@ -483,8 +483,6 @@ def build_param_doc(arg_names, arg_types, arg_descs, remove_dup=True):
for key, type_info, desc in zip(arg_names, arg_types, arg_descs):
if key in param_keys and remove_dup:
continue
if key == 'num_args':
Copy link
Member

Choose a reason for hiding this comment

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

num_args is populated for variable length inputs without users having to worry about it. If we remove this the additional num_args field can confuse users.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok, then i'll just add a note in the description about providing 2 inputs for bilinear sampling

src/operator/nn/upsampling-inl.h Outdated
.describe("Number of inputs to be upsampled. For nearest neighbor "
"upsampling, this can be 1-N; the size of output will be"
"(scale*h_0,scale*w_0) and all other inputs will be upsampled to the"
"same size. For bilinear upsampling this must be 2; 1 input and 1 weight.");
Copy link
Member

Choose a reason for hiding this comment

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

We can just move this information to the description of data field.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok

anirudhacharya
anirudhacharya reviewed Feb 18, 2019
View reviewed changes
tests/python/unittest/test_operator.py Outdated
@@ -1514,6 +1527,20 @@ def test_nearest_upsampling():
check_nearest_upsampling_with_shape(shapes, scale, root_scale)


@with_seed()
def test_bilinear_upsampling():
for root_scale in [2,3]:
Copy link
Member

Choose a reason for hiding this comment

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

you could use itertools.product like here - https://github.com/anirudhacharya/incubator-mxnet/blob/master/tests/python/unittest/test_optimizer.py#L1225

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok

apeforest
apeforest reviewed Feb 19, 2019
View reviewed changes
tests/python/unittest/test_operator.py
arr_grad = {'arg_%d'%i: mx.nd.zeros(shape) for i, shape in zip(range(len(shapes)), shapes)}

up = mx.sym.UpSampling(*[mx.sym.Variable('arg_%d'%i) for i in range(len(shapes))], sample_type='bilinear', scale=root_scale)
def check_bilinear_upsampling_with_shape(data_shape, weight_shape, scale, root_scale, num_filter):
Copy link
Contributor

Choose a reason for hiding this comment

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

Why did you change the unit test if your PR is to fix documentation?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@apeforest the unit test was not being called previously. And based on inputs from the issues in Ref section of the PR description, many users are confused about how to use the Upsampling operator with the bilinear upsampling option. So i thought I could add/modify tests here as well - which would show how to use the operator.
I have updated the PR title - should I open a separate PR instead?

Note: This PR is WIP as an assert has to be added at the end of the test. Based on an offline discussion with @anirudh2290 yesterday, the assert will be similar to what was done for test_deconvolution()

@vandanavk vandanavk changed the title [WIP] Fix documentation for bilinear upsampling [WIP] Fix documentation for bilinear upsampling and add unit test Feb 19, 2019
@vandanavk vandanavk changed the title [WIP] Fix documentation for bilinear upsampling and add unit test Fix documentation for bilinear upsampling and add unit test Mar 16, 2019
@vandanavk
Copy link
Contributor Author

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

@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 Mar 16, 2019
@piyushghai
Copy link
Contributor

@apeforest @anirudhacharya Are you comments on the PR addressed now ? Can this PR be taken forward now ?

@Roshrini
Copy link
Member

@anirudh2290 @apeforest Can you take another look at this PR?

@anirudh2290 anirudh2290 merged commit 3b23c2d into apache:master Apr 17, 2019
kedarbellare pushed a commit to kedarbellare/incubator-mxnet that referenced this pull request Apr 20, 2019
@vandanavk @kedarbellare
Fix documentation for bilinear upsampling and add unit test (apache#1…
a617eb0 
…4035)

* Bilinear upsampling documentation and test

* test trial

* Edit test

* Addressed review comments

* Fix lint error

* Test target shape
haohuanw pushed a commit to haohuanw/incubator-mxnet that referenced this pull request Jun 23, 2019
@vandanavk
Fix documentation for bilinear upsampling and add unit test (apache#1…
f60d93d 
…4035)

* Bilinear upsampling documentation and test

* test trial

* Edit test

* Addressed review comments

* Fix lint error

* Test target shape
@benhe2011 benhe2011 mentioned this pull request Aug 8, 2019
Open
5 tasks
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@apeforest apeforest apeforest left review comments

@anirudhacharya anirudhacharya anirudhacharya left review comments

@anirudh2290 anirudh2290 anirudh2290 approved these changes

@szha szha Awaiting requested review from szha

Assignees
No one assigned
Labels
Operator pr-awaiting-review PR is waiting for code review
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

UpSampling Operator Documentation is incomplete
8 participants
@vandanavk @lupesko @piyushghai @Roshrini @anirudh2290 @anirudhacharya @apeforest @marcoabreu