pytorch
Add support for 'same' and 'valid' for padding in conv
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:
- Fix 3d case (similar to 2d)
- 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.padallows uneven padding, conv layer doesn't) - 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
@facebook-github-bot has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.
@ashkan-software do you know why commit CircleCI tests weren't triggered?
LGTM overall, I can accept if all the tests and linters pass
@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?
@facebook-github-bot has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.
@ffuuugor thanks a lot for your recommendations. I applied them all and we are good to go!
@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
