-
Notifications
You must be signed in to change notification settings - Fork 25.6k
Make ones and zeros's ref accepts variadic size argument #85117
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
Conversation
[ghstack-poisoned]
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/85117
Note: Links to docs will display an error until the docs builds have been completed. ✅ No FailuresAs of commit 4fda299: This comment was automatically generated by Dr. CI and updates every 15 minutes. |
@out_wrapper() | ||
def zeros( | ||
size: ShapeType, | ||
*, |
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.
why did you remove kwarg-only? dtype and following args are kwarg only.
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.
Python syntax actually doesn't allow this: Keyword-only argument separator not allowed after "*parameter"
Coz, arguments after "*parameter" are implicitly kwarg-only...
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.
@overload
def ones(size: _size, *, out: Optional[Tensor]=None, dtype: Optional[_dtype]=None, layout: Optional[_layout]=None, device: Optional[Union[_device, str, None]]=None, pin_memory: Optional[_bool]=False, requires_grad: Optional[_bool]=False) -> Tensor: ...
@overload
def ones(*size: _int, out: Optional[Tensor]=None, dtype: Optional[_dtype]=None, layout: Optional[_layout]=None, device: Optional[Union[_device, str, None]]=None, pin_memory: Optional[_bool]=False, requires_grad: Optional[_bool]=False) -> Tensor: ...
These are the overloads for torch.ones.
So what we have in the doc, def one(*size, *, ...) , is actually invalid
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.
ok cool, can you add some sample or reference input to exercise new code path? Apparently tests were not failing previously even though they should have.
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.
I added the test cases in opinf db for ones and zeros....
However, the test infra is alway normalizing the inputs to a tuple, so technically it's still not testing the non-tuple size case...
I guess if we really want to test the non-tuple case, we need to carve out an code path just for this purpose. Do we really want to do this?
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.
Yeah ok, we can leave it as is.
[ghstack-poisoned]
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.
Cool, thank you!
[ghstack-poisoned]
@pytorchbot merge -g |
@pytorchbot successfully started a merge job. Check the current status here. |
[ghstack-poisoned]
Merge failedReason: New commits were pushed while merging. Please rerun the merge command. Details for Dev Infra teamRaised by workflow job |
@pytorchbot merge -g |
@pytorchbot successfully started a merge job. Check the current status here. |
Hey @SherlockNoMad. |
@pytorchmergebot revert -c nosignal -m "Failed trunk" |
@pytorchbot successfully started a revert job. Check the current status here. |
[ghstack-poisoned]
@pytorchbot merge -g |
@pytorchbot successfully started a merge job. Check the current status here. |
Merge failedReason: 1 additional jobs have failed, first few of them are: trunk Details for Dev Infra teamRaised by workflow job |
@pytorchbot rebase -s |
@pytorchbot successfully started a rebase job. Check the current status here |
[ghstack-poisoned]
Successfully rebased |
@pytorchbot merge -g |
@pytorchbot successfully started a merge job. Check the current status here. |
Hey @SherlockNoMad. |
Pull Request resolved: #85117 Approved by: https://github.com/ngimel, https://github.com/Lezcano
)" This reverts commit 7e5616c. Reverted #85117 on behalf of https://github.com/ZainRizvi due to Failed trunk
Pull Request resolved: #85117 Approved by: https://github.com/ngimel, https://github.com/lezcano
Stack from ghstack (oldest at bottom):