-
Notifications
You must be signed in to change notification settings - Fork 25.7k
Unify the shape notation for all of the pytorch modules #15741
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
|
I don't have any objection to these changes per se, but if there is actually going to be a canonical format for shape annotations so that they are machine readable, I would expect to see (1) a description of what this format is somewhere, and (2) some sort of lint pass which ensures that this format is upheld in the future. |
| - Input: :math:`(\ast_1, N, \ast_2)` where `*` means, any number of additional | ||
| dimensions | ||
| - Output: :math:`(*, N / 2, *)` | ||
| - Output: :math:`(\ast_1, M, \ast_2)` where :math:`M=N/2` |
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.
Any reason you out-of-line declared M here? I think the original text is easier to read, since M is not referenced anywhere else.
|
Can do. My idea was just to have them be all letters or stars, and use subscripts to indicate changed dims, e.g. H_{in} -> H_{out} . However maybe it is better to use something more standard? |
|
I think not putting simple formulas inline would make matters strictly worse. Plus, you didn't even really gain anything, since any hypothetical machine reader would have to grovel for the "where M = ..." line and parse that anyway |
|
Hmm, okay. All I really care about in the shortterm is which dimensions change. It seems like for the vast majority of examples, it is already of the form (N, H_in) where H_in is some formula. I wasn't planning on parsing the formula, but I guess if it is short it can be inline? I guess the right way to do this it literally write the shape formulas in code and turn them into unit tests. Maybe I will do that instead if I have time. |
| Shape: | ||
| - Input: :math:`(*, N, *)` where `*` means, any number of additional | ||
| - Input: :math:`(\ast_1, N, \ast_2)` where `*` means, any number of additional |
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.
What's the difference between \ast and *?
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.
The code is inconsistent and flips between the two. I think \ast renders a bit nicer, particularly with subscripts.
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'm going to err on the side of taking this PR, because it adds some shape info which we didn't have before, which trumps any nits I have about exactly how to format.
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.
@ezyang is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
|
I have been having discussion with the Hasktorch folks about formalizing this type of shape information. We are imagining a syntax kind of between what they are doing and PyContracts (https://andreacensi.github.io/contracts/tour.html#quick-tour) which enforces shape info for numpy. |
|
I came across the C++20 contracts spec which we'll keep in mind as we do this as well -- potentially useful for pytorch devs. |
PR to update the shape notation for all of the torch.nn modules to take a unified form. The goal is to make these definitions machine-readable and those checkable by unifying the style across all of the different modules.