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

[MXNET-867] Pooling1D with "same" padding #12594

Merged
merged 10 commits into from
Sep 25, 2018
Merged

[MXNET-867] Pooling1D with "same" padding #12594

merged 10 commits into from
Sep 25, 2018

Conversation

ChaiBapchya
Copy link
Contributor

Description

MaxPooling 1D added another pooling convention (aka padding type) called "same"

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)
  • To the my best knowledge, examples are either not affected by this change, or have been fixed to be compatible with this change

@ChaiBapchya ChaiBapchya mentioned this pull request Sep 19, 2018
Open
@stu1130
Copy link
Contributor

stu1130 commented Sep 19, 2018

Thanks for your contribution @ChaiBapchya
@mxnet-label-bot[pr-awaiting-review]

@marcoabreu marcoabreu added the pr-awaiting-review PR is waiting for code review label Sep 19, 2018
apeforest
apeforest reviewed Sep 21, 2018
View reviewed changes
src/operator/nn/pooling.cc Outdated
static_cast<float>(dshape[2] + 2 * param.pad[0] -
param.kernel[0]) /
param.stride[0]));
} else {
oshape[2] = static_cast<int>(ceil(
static_cast<float>(dshape[2] + 2 * param.pad[0]) /
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: indentation

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unable to find the indentation issue. Also is there a similar linting tool for python to ensure I don't miss out on those. Do we use pylint?

apeforest
apeforest reviewed Sep 21, 2018
View reviewed changes
tests/python/unittest/test_operator.py
stride = 2
kernel = 2
output_data=mx.nd.Pooling(
input_data,
Copy link
Contributor

@apeforest apeforest Sep 21, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Do you also want to test the case where user selects "same" pad type but also supplies pad values?

@apeforest
Copy link
Contributor

Thanks for making this contribution. LGTM overall. You may also want to update the operator description in operator/nn/pooling.cc since you added a new type "same"

apeforest
apeforest approved these changes Sep 25, 2018
View reviewed changes
Copy link
Contributor

@apeforest apeforest left a comment

Choose a reason for hiding this comment

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

LGTM

@anirudh2290 anirudh2290 merged commit 29ac191 into apache:master Sep 25, 2018
@ChaiBapchya
Copy link
Contributor Author

As discussed with Sina Afrooze and Lin, this is just the first step towards complete solution/fix.

Immediate demand was for Max Pool 1 Dimension to support "same" padding (the way Tensorflow does). Above pull request addresses that.

However, for a complete solution, following things need to be understood

Asymmetric Padding

Feature request is about "Asymmetric padding"
Currently, MXNet supports Symmetric padding
e.g. Input data -> 1,2,3,4 and pad=2
Output data -> 0,0,1,2,3,4,0,0

Why isn't Asymmetric padding supported by CUDnn, MXNet?

Due to misalignment of input-output if padding happens only on one side (left or right alone).
It is called as "Causal Convolution" (only one side pad)

Current work-around

Level of abstraction introduced.
By default, one can use pad parameter while performing Pooling or Convolution which performs symmetric padding. But, if there is a specific use-case for performing asymmetric padding, one has to perform following steps,
Perform independent padding (using pad operator) and then perform Pooling or Convolution as desired.

Future steps

If one has to fully implement asymmetric padding, following changes need to be made

  1. Pooling
  • Add another parameter such as pad_type (because currently, pooling_conv which allows - 'full', 'valid', 'same' has been misused. Original meaning of pooling_conv was not supposed to take care of "type of padding"
  1. Convolution
  • Incorporate asymmetric padding for the same

@ChaiBapchya ChaiBapchya deleted the max_pool_same_padding branch September 25, 2018 19:10
@anirudhacharya
Copy link
Member

anirudhacharya commented Dec 6, 2018 •
edited
Loading

@ChaiBapchya can you create and issue for the future steps, giving the full description as in the above comment? Issues will be tracked, the above comment could get lost.

@ChaiBapchya
Copy link
Contributor Author

Sure. Thanks for suggesting :)

@ChaiBapchya ChaiBapchya mentioned this pull request Dec 7, 2018
Open
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@anirudh2290 anirudh2290 anirudh2290 approved these changes

@haojin2 haojin2 haojin2 left review comments

@access2rohit access2rohit access2rohit left review comments

@apeforest apeforest apeforest approved these changes

Assignees
No one assigned
Labels
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.
8 participants
@ChaiBapchya @stu1130 @apeforest @anirudhacharya @anirudh2290 @haojin2 @access2rohit @marcoabreu