-
Notifications
You must be signed in to change notification settings - Fork 340
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
Fix unfold2d and add test #443
Conversation
@alexandresablayrolles has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
@@ -130,7 +130,8 @@ def unfold2d( | |||
W_effective = ( | |||
W + 2 * padding[1] - (kernel_size[1] + (kernel_size[1] - 1) * (dilation[1] - 1)) | |||
) // stride[1] + 1 | |||
input = F.pad(input, (padding[0], padding[0], padding[1], padding[1])) | |||
# F.pad's first argument is the padding of the *last* dimension |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🙃
Please fix lint before merging. |
Looks great, thanks for the fix! Can we also add more inputs to |
@alexandresablayrolles has updated the pull request. You must reimport the pull request before landing. |
@alexandresablayrolles has updated the pull request. You must reimport the pull request before landing. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. LGTM. Please isort
before merging.
@alexandresablayrolles has updated the pull request. You must reimport the pull request before landing. |
@alexandresablayrolles has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
Fixes #442.
TL;DR: F.pad has an unintuitive syntax (we need to indicated padding from last dimensions to first dimensions).
This PR fixes that in unfold2d and adds tests with non-symmetric pad/dilation/kernel_size/stride.