Skip to content

Add support for 'same' and 'valid' for padding in conv #451

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

Add support for 'same' and 'valid' for padding in conv #451

wants to merge 1 commit into from

Conversation

@ashkan-software
Copy link
Contributor

Differential Revision: D37811390

@facebook-github-bot facebook-github-bot added CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. fb-exported labels Jul 13, 2022
@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D37811390

@ashkan-software ashkan-software marked this pull request as draft July 13, 2022 07:50
@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D37811390

ffuuugor pushed a commit to ffuuugor/opacus that referenced this pull request Jul 22, 2022
@ashkan-software @facebook-github-bot
Add support for 'same' and 'valid' for padding in conv (meta-pytorch#451
325cefc 
)

Summary: Pull Request resolved: meta-pytorch#451

Differential Revision: D37811390

fbshipit-source-id: 589ffe6c043e5566ed32dcca9cf853fb7cfc72cf
@ffuuugor
Copy link
Contributor

So the reason for the failing tests in that padding="same" introduces uneven padding, which previously wasn't supported by either PyTorch or opacus.

All numerical values of padding in convolution layers pad equally from both sides, which we assumed to be the case when writing grad sampler for opacus.
With padding="same" it's possible to have uneven padding: if total padding length is odd number, the padding on the right will be larger by one.

I've prototyped the solution in #456 - it's based on your commit, I've fixed 1d and 2d cases. Still pending is:

  1. Fix 3d case (similar to 2d)
  2. Run benchmarks. I call for F.pad() in all cases, regardless of the input parameters. This is the most straightforward option, but might not be the most efficient. It's possible that doing padding inside conv layer is more efficient - if that's true, we'd want to add an extra if-statement and only do padding manually if required (F.pad allows uneven padding, conv layer doesn't)
  3. Ideally, we'd like this to be supported by ExpandedWeights too. So we'd need to talk to @samdow about adapting this changes in core pytorch code

@ashkan-software ashkan-software mentioned this pull request Jul 29, 2022
Closed
@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D37811390

@facebook-github-bot
Copy link
Contributor

@facebook-github-bot has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@ffuuugor
Copy link
Contributor

@ashkan-software do you know why commit CircleCI tests weren't triggered?
LGTM overall, I can accept if all the tests and linters pass

@ffuuugor
Copy link
Contributor

@ashkan-software For ExpandedWeights tests failures, I don't think they should be blocking for merging this. For now you can disable this particular combination of input arguments (we have ew_compatible parameter in _run method for this).
And add a #TODO to the code so we remember to enable it later when EW starts supporting it.
Additionally, it'd be great if you add this info to grad_sample/README.md, where we document discrepancies between hook-based and ew-based GradSampleModules.

@samdow, do we have an issue created for this, so we can link it for future tracking?

@ashkan-software ashkan-software marked this pull request as ready for review August 11, 2022 01:02
@ashkan-software @facebook-github-bot
Add support for 'same' and 'valid' for padding in conv (#451)
da87296 
Summary: Pull Request resolved: #451

Differential Revision: D37811390

Pulled By: ashkan-software

fbshipit-source-id: 1bbf21c9dffba9dc1fe776a95f9ec8283ad42a8d
@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D37811390

@facebook-github-bot
Copy link
Contributor

@facebook-github-bot has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@ashkan-software
Copy link
Contributor Author

@ffuuugor thanks a lot for your recommendations. I applied them all and we are good to go!

@samdow
Copy link

samdow commented Aug 11, 2022

@samdow, do we have an issue created for this, so we can link it for future tracking?

No but I'm planning to add this today thanks to talk with @ashkan-software! If I don't get to it today, I'll file an issue 😄 Thanks for flagging this and doing the heavy lifting of the implementation

@samdow samdow mentioned this pull request Aug 12, 2022
Closed
pytorchmergebot pushed a commit to pytorch/pytorch that referenced this pull request Aug 16, 2022
@pytorchmergebot
[Expanded Weights] add 'same' and 'valid' padding support (#83345)
6d34ccc 
Co-authored-by: Ashkan <yousefpour@fb.com>

Adds "same" and "valid" padding support, as Opacus (well @ashkan-software) did meta-pytorch/opacus#451

Basics of it are this:
- during forward pass, if there's "same" padding, we manually pad the input (NB: this will cause a small perf hit, haven't benchmarked yet)
- during backward pass, the gradient wrt input needs to be cut down to the correct size if the original padding was same (conv_transpose doesn't accept string padding). Because conv_transpose will give us a gradient wrt the padded shape, we cut down the gradient to the correct size (we know how much padding we added to the left and right)
- then, for the per sample gradients wrt weights, the input is already padded so neither the unfold nor group convolution have any padding
Pull Request resolved: #83345
Approved by: https://github.com/zou3519
pytorchmergebot pushed a commit to pytorch/pytorch that referenced this pull request Aug 16, 2022
@pytorchmergebot
[Expanded Weights] add 'same' and 'valid' padding support (#83345)
3a511e8 
Co-authored-by: Ashkan <yousefpour@fb.com>

Adds "same" and "valid" padding support, as Opacus (well @ashkan-software) did meta-pytorch/opacus#451

Basics of it are this:
- during forward pass, if there's "same" padding, we manually pad the input (NB: this will cause a small perf hit, haven't benchmarked yet)
- during backward pass, the gradient wrt input needs to be cut down to the correct size if the original padding was same (conv_transpose doesn't accept string padding). Because conv_transpose will give us a gradient wrt the padded shape, we cut down the gradient to the correct size (we know how much padding we added to the left and right)
- then, for the per sample gradients wrt weights, the input is already padded so neither the unfold nor group convolution have any padding
Pull Request resolved: #83345
Approved by: https://github.com/zou3519
facebook-github-bot pushed a commit to pytorch/pytorch that referenced this pull request Aug 17, 2022
@ashkan-software @facebook-github-bot
[Expanded Weights] add 'same' and 'valid' padding support (#83345) (#…
1d39213 
…83345)

Summary:

Adds "same" and "valid" padding support, as Opacus (well ashkan-software) did meta-pytorch/opacus#451

Basics of it are this:
- during forward pass, if there's "same" padding, we manually pad the input (NB: this will cause a small perf hit, haven't benchmarked yet)
- during backward pass, the gradient wrt input needs to be cut down to the correct size if the original padding was same (conv_transpose doesn't accept string padding). Because conv_transpose will give us a gradient wrt the padded shape, we cut down the gradient to the correct size (we know how much padding we added to the left and right)
- then, for the per sample gradients wrt weights, the input is already padded so neither the unfold nor group convolution have any padding

Pull Request resolved: #83345
Approved by: https://github.com/zou3519

Test Plan: contbuild & OSS CI, see https://hud.pytorch.org/commit/pytorch/pytorch/3a511e83549d86ce9a2a8de2b64be340d2a23e4e

Reviewed By: seemethere, atalman

Differential Revision: D38770030

Pulled By: samdow

fbshipit-source-id: 8be29c56ee84dce49a9fd765d7005c647b10862d

Co-authored-by: Ashkan <yousefpour@fb.com>
ffuuugor
ffuuugor approved these changes Aug 18, 2022
View reviewed changes
Copy link
Contributor

@ffuuugor ffuuugor left a comment

Choose a reason for hiding this comment

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

Looks great, thanks a lot for the amazing work!

opacus/tests/grad_samples/conv2d_test.py
groups=groups,
)
self.run_test(x, conv, batch_first=True, atol=10e-5, rtol=10e-4)
is_ew_compatible = padding != 'same' # TODO add support for padding = 'same' with EW
Copy link
Contributor

Choose a reason for hiding this comment

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

does "valid" padding work fine with EW?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good question. It should since it's basically zero padding. But let me run a test locally and verify this. Good catch!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

well, actually on line 37 of this file we have unit tests for both "valid" and "same". So it should work just fine 😎

@ashkan-software ashkan-software closed this by deleting the head repository Aug 25, 2022
@ffuuugor ffuuugor added this to the 1.2.0 milestone Aug 26, 2022
@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D37811390

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@ffuuugor ffuuugor ffuuugor approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. fb-exported

Projects

None yet

Milestone

1.2.0

Development

Successfully merging this pull request may close these issues.

4 participants

@ashkan-software @facebook-github-bot @ffuuugor @samdow