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 error checking for padding modules #106147

Conversation

@pytorch-bot
Copy link

pytorch-bot bot commented Jul 27, 2023

🔗 Helpful Links

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

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

✅ No Failures

As of commit c2c0b23:
💚 Looks good so far! There are no failures yet. 💚

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

@mikaylagawarecki mikaylagawarecki marked this pull request as ready for review July 27, 2023 19:54
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.

Sounds good!
Testing?

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.

Thanks for the test!

@mikaylagawarecki mikaylagawarecki added the ciflow/trunk Trigger trunk jobs on your pull request label Jul 31, 2023
# Constant Pad 1D with single dimension input
helper((16), (1, 2), nn.ConstantPad1d)
# Constant Pad with single dimension input
helper_functional((16), (1, 2), 'constant')
Copy link
Contributor Author

Choose a reason for hiding this comment

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

nn.functional.pad allows constant padding for arbitrary dimensions, which is different from the other modes replicate, reflect and circular (see docs), however, nn.ConstantPad{1,2,3}d only allow 2/3D, 3/4D and 4/5D inputs respectively based on the docs (e.g. here for 1D). Changing the cases where input shapes are invalid according the docs to test the functional variant here

pytorchmergebot pushed a commit that referenced this pull request Aug 1, 2023
…06148)

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

Pull Request resolved: #106148
Approved by: https://github.com/albanD
ghstack dependencies: #106325, #106147
@jeanschmidt
Copy link
Contributor

@pytorchbot revert -m "sadly it is breaking internal builds, and I can't coordinate a FF due to timezone differences" -c nosignal

@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

Reverting PR 106147 failed

Reason: Comment with id 1661870454 not found

Details for Dev Infra team Raised by workflow job

@jeanschmidt
Copy link
Contributor

@pytorchbot revert -m "sadly it is breaking internal builds, and I can't coordinate a FF due to timezone differences" -c nosignal

@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
This reverts commit 0547b62.

Reverted #106147 on behalf of https://github.com/jeanschmidt due to sadly it is breaking internal builds, and I can't coordinate a FF due to timezone differences ([comment](#106147 (comment)))
@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.

@malfet
Copy link
Contributor

malfet commented Aug 2, 2023

@jeanschmidt nosignal is not a correct category for revert due to internal failures, it should be ghfirst

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/135/head branch August 4, 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]
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 Reverted topic: not user facing topic category
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Some problems about dimension with Pad Layers
5 participants