KEMBAR78
Unify the shape notation for all of the pytorch modules by srush · Pull Request #15741 · pytorch/pytorch · GitHub
Skip to content

Conversation

@srush
Copy link
Contributor

@srush srush commented Jan 4, 2019

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.

@ezyang
Copy link
Contributor

ezyang commented Jan 7, 2019

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`
Copy link
Contributor

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.

@srush
Copy link
Contributor Author

srush commented Jan 7, 2019

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?

@ezyang
Copy link
Contributor

ezyang commented Jan 7, 2019

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

@srush
Copy link
Contributor Author

srush commented Jan 7, 2019

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
Copy link
Contributor

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 *?

Copy link
Contributor Author

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.

Copy link
Contributor

@ezyang ezyang left a 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.

Copy link
Contributor

@facebook-github-bot facebook-github-bot left a 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.

@srush
Copy link
Contributor Author

srush commented Jan 18, 2019

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.

hasktorch/hasktorch#139

@ezyang
Copy link
Contributor

ezyang commented Jan 18, 2019

Thanks for the notes @srush. Also cc'ing @zdevito. If we keep moving in this direction, it would also be helpful if we can adjust the test suite to test these shape contracts when we do other tests for given input sizes.

@stites
Copy link

stites commented Jan 28, 2019

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants