-
Notifications
You must be signed in to change notification settings - Fork 26.3k
Add document of function torch.as_strided #22842
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
facebook-github-bot
left a comment
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.
@soumith is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
torch/_torch_docs.py
Outdated
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 think it is important to mention that this creates a view of an existing tensor. :)
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.
You are right. Will Create a view of an existing torch.Tensor ... be better?
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.
Yes, that sounds great! Thank you!
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.
Also, could you also add an entry to the method version of it at https://github.com/pytorch/pytorch/blob/f2f80744be347fab6921fc55181b55dc86c7e07b/torch/_tensor_docs.py? It should be easy and could just refer to torch.as_strided.
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.
Actually I think torch.as_strided probably should not exist. Only the method version should, similar to resize.
cc @gchanan who probably should have better opinion on 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.
I think it's fine, torch.as_tensor exists (because it needs to), so other as_ functions seem like fair game.
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. I will update both soon.
ade9299 to
e0fe038
Compare
ssnl
left a comment
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.
Thank you!
zou3519
left a comment
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.
Wait, I thought it was intentional that torch.as_strided doesn't have documentation because it is dangerous and that users shouldn't be using it. Can we at least add a big red warning that this is dangerous?
I think we can do that. Do we need to write some specific reasons why this is dangerous? |
Things we should write:
|
|
I think that is is not that dangerous. You can’t access data outside the underlying storage. The important thing is to mention that this returns a view. We can check what np says about their counterpart in the docs. |
https://docs.scipy.org/doc/numpy-1.13.0/reference/generated/numpy.lib.stride_tricks.as_strided.html for reference @kianasun |
e0fe038 to
fd3839a
Compare
|
@zou3519 Updated. :) |
torch/_torch_docs.py
Outdated
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.
@ssnl says this isn't possible anymore. I played around with as_strided as well and noticed there are indeed checks, so maybe it isn't applicable anymore
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. I will remove the first paragraph.
torch/_torch_docs.py
Outdated
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.
You should only use this function if you know what you're doing.
I know I suggested this, but this sounds bad. Something like the following would be better:
"Many PyTorch functions that return a view on a tensor are internally implemented with this function. Those functions, like torch.expand, are easier to read and are therefore more advisable to use"
fd3839a to
132d578
Compare
xuhdev
left a comment
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.
There is also an inplace version as_strided_, you probably wanna document it as well (in docs/source/tensors.rst). Document test skipping should also be deleted:
Lines 196 to 197 in 7d055c2
| 'as_strided', | |
| 'as_strided_', |
|
I wouldn't document the inplace version.. |
|
@ssnl Why don't we unexpose that function if we do not want user to use it (this should probably discussed in a different thread)? Leaving a function undocumented doesn't prevent adventurous users from using it. |
|
@xuhdev We should hide it IMHO, just like many other things. It's just that no one has done it. It would probably require a deprecation cycle and fix the internal callsites. |
facebook-github-bot
left a comment
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.
@soumith has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
132d578 to
5d88cfc
Compare
facebook-github-bot
left a comment
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.
@soumith has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
|
@pytorchbot rebase this please |
facebook-github-bot
left a comment
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.
@soumith has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
|
@pytorchbot rebase this please |
facebook-github-bot
left a comment
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.
@soumith is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
|
@pytorchbot rebase this please |
facebook-github-bot
left a comment
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.
@soumith has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
facebook-github-bot
left a comment
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.
@soumith has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
Documentation of
torch.as_stridedandTensor.as_stridedis missing. As mentioned in #9886