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

Add nn.CircularPad{*}d for consistency + fix no_batch_dim support #106148

Conversation

mikaylagawarecki
Copy link
Contributor

@mikaylagawarecki mikaylagawarecki commented Jul 27, 2023

Fixes #105749 #95320

(tldr is that input should always be [N, C, H, (W, D]) where only H, W and D dimensions get circular padding, so the 2D case where user wants both dimensions to be padded --> they should .unsqueeze(0) (as is the case for Reflection/ReplicationPad) but we didn't document this for circular padding. This seems to be the old docstring that was somehow lost.

Fixes no_batch_dim support #104860

  • Adds missing documentation for circular padding
  • Adds missing CircularPad modules
  • Migrates legacy test_nn tests from circular padding to ModuleInfo
  • Adds no_batch_dim support + sample inputs that test this

Stack from ghstack (oldest at bottom):

cc @albanD @mruberry @jbschlosser @walterddr

@pytorch-bot
Copy link

pytorch-bot bot commented Jul 27, 2023

🔗 Helpful Links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/106148

Note: Links to docs will display an error until the docs builds have been completed.

✅ 2 Unrelated Failures

As of commit fa27baf:

UNSTABLE - The following jobs failed but were likely due to flakiness present on trunk and has been marked as unstable:

This comment was automatically generated by Dr. CI and updates every 15 minutes.

mikaylagawarecki added a commit that referenced this pull request Jul 27, 2023
ghstack-source-id: c34659c5956bbd8da30ff8edfb96ba98ef4c696f
Pull Request resolved: #106148
…support"

Fixes #105749 #95320 

(tldr is that input should always be `[N, C, H, (W, D])` where only H, W and D dimensions get circular padding, so the 2D case where user wants both dimensions to be padded --> they should `.unsqueeze(0)` (as is the case for `Reflection/ReplicationPad`) but we didn't document this for circular padding

Fixes no_batch_dim support #104860


- Adds missing documentation for circular padding
- Adds missing CircularPad modules
- Migrates legacy test_nn tests from circular padding to ModuleInfo
- Adds no_batch_dim support + sample inputs that test this




[ghstack-poisoned]
mikaylagawarecki added a commit that referenced this pull request Jul 27, 2023
ghstack-source-id: de485a292145a993287b194d1c4c523c8fd14722
Pull Request resolved: #106148
@mikaylagawarecki mikaylagawarecki marked this pull request as ready for review July 27, 2023 19:54
…support"

Fixes #105749 #95320 

(tldr is that input should always be `[N, C, H, (W, D])` where only H, W and D dimensions get circular padding, so the 2D case where user wants both dimensions to be padded --> they should `.unsqueeze(0)` (as is the case for `Reflection/ReplicationPad`) but we didn't document this for circular padding

Fixes no_batch_dim support #104860


- Adds missing documentation for circular padding
- Adds missing CircularPad modules
- Migrates legacy test_nn tests from circular padding to ModuleInfo
- Adds no_batch_dim support + sample inputs that test this




[ghstack-poisoned]
mikaylagawarecki added a commit that referenced this pull request Jul 27, 2023
ghstack-source-id: 5c1f88340ac04a2d1226591377f4f8dc1924cae9
Pull Request resolved: #106148
Copy link
Collaborator

@albanD albanD left a comment

Choose a reason for hiding this comment

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

Very nice update to the kernel and love to see more testing!

torch/testing/_internal/common_modules.py Outdated Show resolved Hide resolved
torch/nn/modules/padding.py Outdated Show resolved Hide resolved
…support"

Fixes #105749 #95320 

(tldr is that input should always be `[N, C, H, (W, D])` where only H, W and D dimensions get circular padding, so the 2D case where user wants both dimensions to be padded --> they should `.unsqueeze(0)` (as is the case for `Reflection/ReplicationPad`) but we didn't document this for circular padding. [This seems to be the old docstring](https://github.com/pytorch/pytorch/blob/277b05014aced075337d6f84a535bd5150aabebb/torch/nn/functional.py#L4689) that was somehow lost.

Fixes no_batch_dim support #104860


- Adds missing documentation for circular padding
- Adds missing CircularPad modules
- Migrates legacy test_nn tests from circular padding to ModuleInfo
- Adds no_batch_dim support + sample inputs that test this




[ghstack-poisoned]
mikaylagawarecki added a commit that referenced this pull request Jul 31, 2023
ghstack-source-id: 3d85cb966098c3c9c557b67813d28efcabdda697
Pull Request resolved: #106148
…support"

Fixes #105749 #95320 

(tldr is that input should always be `[N, C, H, (W, D])` where only H, W and D dimensions get circular padding, so the 2D case where user wants both dimensions to be padded --> they should `.unsqueeze(0)` (as is the case for `Reflection/ReplicationPad`) but we didn't document this for circular padding. [This seems to be the old docstring](https://github.com/pytorch/pytorch/blob/277b05014aced075337d6f84a535bd5150aabebb/torch/nn/functional.py#L4689) that was somehow lost.

Fixes no_batch_dim support #104860


- Adds missing documentation for circular padding
- Adds missing CircularPad modules
- Migrates legacy test_nn tests from circular padding to ModuleInfo
- Adds no_batch_dim support + sample inputs that test this




[ghstack-poisoned]
mikaylagawarecki added a commit that referenced this pull request Jul 31, 2023
ghstack-source-id: 3a7ed23df9172f5d2b175853a94a077d7b512d6a
Pull Request resolved: #106148
Copy link
Collaborator

@albanD albanD left a comment

Choose a reason for hiding this comment

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

SGTM

@mikaylagawarecki mikaylagawarecki added ciflow/trunk Trigger trunk jobs on your pull request module: nn Related to torch.nn release notes: nn release notes category topic: improvements topic category and removed module: nn Related to torch.nn labels Jul 31, 2023
…support"

Fixes #105749 #95320 

(tldr is that input should always be `[N, C, H, (W, D])` where only H, W and D dimensions get circular padding, so the 2D case where user wants both dimensions to be padded --> they should `.unsqueeze(0)` (as is the case for `Reflection/ReplicationPad`) but we didn't document this for circular padding. [This seems to be the old docstring](https://github.com/pytorch/pytorch/blob/277b05014aced075337d6f84a535bd5150aabebb/torch/nn/functional.py#L4689) that was somehow lost.

Fixes no_batch_dim support #104860


- Adds missing documentation for circular padding
- Adds missing CircularPad modules
- Migrates legacy test_nn tests from circular padding to ModuleInfo
- Adds no_batch_dim support + sample inputs that test this




cc albanD mruberry jbschlosser walterddr

[ghstack-poisoned]
mikaylagawarecki added a commit that referenced this pull request Jul 31, 2023
ghstack-source-id: 21212ba64b7c2ec1aef19c6c92dc4e6aaf361958
Pull Request resolved: #106148
…support"

Fixes #105749 #95320 

(tldr is that input should always be `[N, C, H, (W, D])` where only H, W and D dimensions get circular padding, so the 2D case where user wants both dimensions to be padded --> they should `.unsqueeze(0)` (as is the case for `Reflection/ReplicationPad`) but we didn't document this for circular padding. [This seems to be the old docstring](https://github.com/pytorch/pytorch/blob/277b05014aced075337d6f84a535bd5150aabebb/torch/nn/functional.py#L4689) that was somehow lost.

Fixes no_batch_dim support #104860


- Adds missing documentation for circular padding
- Adds missing CircularPad modules
- Migrates legacy test_nn tests from circular padding to ModuleInfo
- Adds no_batch_dim support + sample inputs that test this




cc albanD mruberry jbschlosser walterddr

[ghstack-poisoned]
mikaylagawarecki added a commit that referenced this pull request Jul 31, 2023
ghstack-source-id: 1c6ac41aea84e103f881358f66fb2bc7809cd761
Pull Request resolved: #106148
@mikaylagawarecki
Copy link
Contributor Author

@pytorchbot merge

…support"

Fixes #105749 #95320 

(tldr is that input should always be `[N, C, H, (W, D])` where only H, W and D dimensions get circular padding, so the 2D case where user wants both dimensions to be padded --> they should `.unsqueeze(0)` (as is the case for `Reflection/ReplicationPad`) but we didn't document this for circular padding. [This seems to be the old docstring](https://github.com/pytorch/pytorch/blob/277b05014aced075337d6f84a535bd5150aabebb/torch/nn/functional.py#L4689) that was somehow lost.

Fixes no_batch_dim support #104860


- Adds missing documentation for circular padding
- Adds missing CircularPad modules
- Migrates legacy test_nn tests from circular padding to ModuleInfo
- Adds no_batch_dim support + sample inputs that test this




cc albanD mruberry jbschlosser walterddr

[ghstack-poisoned]
mikaylagawarecki added a commit that referenced this pull request Aug 1, 2023
ghstack-source-id: 5ef5cc48081275f0bbfb9368aa41da94372851b9
Pull Request resolved: #106148
@pytorchmergebot
Copy link
Collaborator

Merge failed

Reason: New commits were pushed while merging. Please rerun the merge command.

Details for Dev Infra team Raised by workflow job

@mikaylagawarecki
Copy link
Contributor Author

@pytorchbot merge

@pytorchmergebot
Copy link
Collaborator

Merge started

Your change will be merged once all checks pass (ETA 0-4 Hours).

Learn more about merging in the wiki.

Questions? Feedback? Please reach out to the PyTorch DevX Team

Advanced Debugging
Check the merge workflow status
here

@pytorchmergebot
Copy link
Collaborator

Merge failed

Reason: Command git -C /home/runner/work/pytorch/pytorch cherry-pick -x 502e6e49d32d1bcd25238316f1c9823c4377fca4 returned non-zero exit code 1

The previous cherry-pick is now empty, possibly due to conflict resolution.
If you wish to commit it anyway, use:

    git commit --allow-empty

Otherwise, please use 'git cherry-pick --skip'
On branch main
Your branch is ahead of 'origin/main' by 1 commit.
  (use "git push" to publish your local commits)

You are currently cherry-picking commit 502e6e49d32.
  (all conflicts fixed: run "git cherry-pick --continue")
  (use "git cherry-pick --skip" to skip this patch)
  (use "git cherry-pick --abort" to cancel the cherry-pick operation)

nothing to commit, working tree clean
Details for Dev Infra team Raised by workflow job

@mikaylagawarecki
Copy link
Contributor Author

@pytorchbot merge

@pytorchmergebot
Copy link
Collaborator

Merge started

Your change will be merged once all checks pass (ETA 0-4 Hours).

Learn more about merging in the wiki.

Questions? Feedback? Please reach out to the PyTorch DevX Team

Advanced Debugging
Check the merge workflow status
here

@malfet
Copy link
Contributor

malfet commented Aug 2, 2023

@pytorchbot revert -m "Reverting as dependent PR #106147 was reverted as well" -c weird

@pytorchmergebot
Copy link
Collaborator

@pytorchbot successfully started a revert job. Check the current status here.
Questions? Feedback? Please reach out to the PyTorch DevX Team

@pytorchmergebot
Copy link
Collaborator

@mikaylagawarecki your PR has been successfully reverted.

pytorchmergebot added a commit that referenced this pull request Aug 2, 2023
…port (#106148)"

This reverts commit 87d2536.

Reverted #106148 on behalf of https://github.com/malfet due to Reverting as dependent PR #106147 was reverted as well ([comment](#106148 (comment)))
@facebook-github-bot facebook-github-bot deleted the gh/mikaylagawarecki/136/head branch August 4, 2023 14:17
@mikaylagawarecki mikaylagawarecki restored the gh/mikaylagawarecki/136/head branch August 4, 2023 20:35
mikaylagawarecki added a commit that referenced this pull request Aug 4, 2023
mikaylagawarecki added a commit that referenced this pull request Aug 4, 2023
…port (#106148)"

ghstack-source-id: d99b1f2947c2ecfb1807e3b92b8240feb630db8d
Pull Request resolved: #106632
@facebook-github-bot facebook-github-bot deleted the gh/mikaylagawarecki/136/head branch August 5, 2023 14:17
mikaylagawarecki added a commit that referenced this pull request Aug 7, 2023
…tch_dim support (#106148)""

Previous one was reverted because the PR stacked under which added error-checking to Pad variants #106147 was reverted as internally some people pass 2D inputs to ZeroPad2d (which should actually take 3d or 4d inputs :) but there wasn't actually anything this PR was breaking according to my understanding




[ghstack-poisoned]
mikaylagawarecki added a commit that referenced this pull request Aug 7, 2023
…port (#106148)"

ghstack-source-id: 484919d427293464ca8163d22ba2d633f47ad2c9
Pull Request resolved: #106632
pytorchmergebot pushed a commit that referenced this pull request Aug 7, 2023
…port (#106148)" (#106632)

Previous one was reverted because the PR stacked under which added error-checking to Pad variants #106147 was reverted as internally some people pass 2D inputs to ZeroPad2d (which should actually take 3d or 4d inputs :) but there wasn't actually anything this PR was breaking according to my understanding

Pull Request resolved: #106632
Approved by: https://github.com/albanD
Cyril-Anto pushed a commit to Cyril-Anto/pytorch that referenced this pull request Aug 17, 2023
…port (pytorch#106148)" (pytorch#106632)

Previous one was reverted because the PR stacked under which added error-checking to Pad variants pytorch#106147 was reverted as internally some people pass 2D inputs to ZeroPad2d (which should actually take 3d or 4d inputs :) but there wasn't actually anything this PR was breaking according to my understanding

Pull Request resolved: pytorch#106632
Approved by: https://github.com/albanD
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ciflow/trunk Trigger trunk jobs on your pull request Merged release notes: nn release notes category Reverted topic: improvements topic category
Projects
None yet
4 participants