Skip to content

Conversation

zou3519
Copy link
Contributor

@zou3519 zou3519 commented Jun 18, 2019

Stack from ghstack:

  • empty, ones, zeros, rand, randn

Test Plan

  • new tests [namedtensor ci]

gh-metadata: pytorch pytorch 21919 gh/zou3519/56/head

Differential Revision: D15940549

- empty, ones, zeros, rand, randn

Test Plan
- new tests [namedtensor ci]
zou3519 added 3 commits June 18, 2019 13:49
Added names= arg for some creation functions

- empty, ones, zeros, rand, randn

Test Plan
- new tests [namedtensor ci]

gh-metadata: pytorch pytorch 21919 gh/zou3519/56/head
Added names= arg for some creation functions

- empty, ones, zeros, rand, randn

Test Plan
- new tests [namedtensor ci]

gh-metadata: pytorch pytorch 21919 gh/zou3519/56/head
Added names= arg for some creation functions

- empty, ones, zeros, rand, randn

Test Plan
- new tests [namedtensor ci]

gh-metadata: pytorch pytorch 21919 gh/zou3519/56/head

- func: scalar_tensor(Scalar s, *, ScalarType? dtype=None, Layout? layout=None, Device? device=None, bool? pin_memory=None) -> Tensor

- func: rand(int[] size, *, Dimname[]? names, ScalarType? dtype=None, Layout? layout=None, Device? device=None, bool? pin_memory=None) -> Tensor
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think everything after the * is optional and kwarg only, is there a reason we can't just provide a default None value and merge this with the existing interface below?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, there is a reason and it is complicated. The short answer is that this is the easiest way to implement it.

Long answer: We've got two rand declarations in native_functions.yaml (without the Generator arg):

- func: rand(int[] size, *, ScalarType? dtype=None, Layout? layout=None, Device? device=None, bool? pin_memory=None) -> Tensor
- func: rand(int[] size, *, Tensor(a!) out) -> Tensor(a!)

These get merged together to produce one scheme string for the python arg parser:

    "rand(IntArrayRef size, *, Tensor out=None, ScalarType dtype=None, Layout layout=torch.strided, Device device=None, bool pin_memory=False, bool requires_grad=False)",

Now, my claim is that it doesn't make sense to add a names= argument when an out tensor is provided. If there were such an arg, it could either

  • (1) check that the out tensor has the correct names. In this case, the user would always have to specify the correct names which is bad because it's verbose and unnecessary.
  • (2) override the names of the out tensor: this is weird, especially if names=None by default.

Because it doesn't make sense, rand(int[] size, *, Dimname[]? names, ScalarType? dtype=None, Layout? layout=None, Device? device=None, bool? pin_memory=None) -> Tensor
should be a separate schema that isn't combined with an out= argument.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah weird, OK

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's weird and I don't like it because it means we need 2x the schemas and that it's easy for the schemas to go out of sync. But I don't know of how else we can do this.

#ifdef NAMEDTENSOR_ENABLED
Tensor ones(
IntArrayRef size,
at::optional<DimnameList> names,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know we don't plan to support an ordered-dict option because of python2, but is there any chance we'll want to support an option to construct named tensors in C++ using an ordered map? or would the syntax be just as unwieldy as python2?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are no plans for named tensor support in C++ in the near future.

zou3519 added 4 commits June 21, 2019 07:29
Added names= arg for some creation functions

- empty, ones, zeros, rand, randn

Test Plan
- new tests [namedtensor ci]

gh-metadata: pytorch pytorch 21919 gh/zou3519/56/head
Added names= arg for some creation functions

- empty, ones, zeros, rand, randn

Test Plan
- new tests [namedtensor ci]

gh-metadata: pytorch pytorch 21919 gh/zou3519/56/head
Added names= arg for some creation functions

- empty, ones, zeros, rand, randn

Test Plan
- new tests [namedtensor ci]

gh-metadata: pytorch pytorch 21919 gh/zou3519/56/head
Added names= arg for some creation functions

- empty, ones, zeros, rand, randn

Test Plan
- new tests [namedtensor ci]

gh-metadata: pytorch pytorch 21919 gh/zou3519/56/head
zou3519 added 4 commits June 21, 2019 09:55
Added names= arg for some creation functions

- empty, ones, zeros, rand, randn

Test Plan
- new tests [namedtensor ci]

gh-metadata: pytorch pytorch 21919 gh/zou3519/56/head
Added names= arg for some creation functions

- empty, ones, zeros, rand, randn

Test Plan
- new tests [namedtensor ci]

gh-metadata: pytorch pytorch 21919 gh/zou3519/56/head
Added names= arg for some creation functions

- empty, ones, zeros, rand, randn

Test Plan
- new tests [namedtensor ci]

gh-metadata: pytorch pytorch 21919 gh/zou3519/56/head
Added names= arg for some creation functions

- empty, ones, zeros, rand, randn

Test Plan
- new tests [namedtensor ci]

gh-metadata: pytorch pytorch 21919 gh/zou3519/56/head
Added names= arg for some creation functions

- empty, ones, zeros, rand, randn

Test Plan
- new tests [namedtensor ci]

gh-metadata: pytorch pytorch 21919 gh/zou3519/56/head
zou3519 added 5 commits June 24, 2019 12:19
Added names= arg for some creation functions

- empty, ones, zeros, rand, randn

Test Plan
- new tests [namedtensor ci]

gh-metadata: pytorch pytorch 21919 gh/zou3519/56/head
Added names= arg for some creation functions

- empty, ones, zeros, rand, randn

Test Plan
- new tests [namedtensor ci]

gh-metadata: pytorch pytorch 21919 gh/zou3519/56/head
Added names= arg for some creation functions

- empty, ones, zeros, rand, randn

Test Plan
- new tests [namedtensor ci]

gh-metadata: pytorch pytorch 21919 gh/zou3519/56/head
Added names= arg for some creation functions

- empty, ones, zeros, rand, randn

Test Plan
- new tests [namedtensor ci]

gh-metadata: pytorch pytorch 21919 gh/zou3519/56/head
Added names= arg for some creation functions

- empty, ones, zeros, rand, randn

Test Plan
- new tests [namedtensor ci]

gh-metadata: pytorch pytorch 21919 gh/zou3519/56/head
Added names= arg for some creation functions

- empty, ones, zeros, rand, randn

Test Plan
- new tests [namedtensor ci]

gh-metadata: pytorch pytorch 21919 gh/zou3519/56/head
zou3519 added 4 commits June 25, 2019 13:03
Added names= arg for some creation functions

- empty, ones, zeros, rand, randn

Test Plan
- new tests [namedtensor ci]

gh-metadata: pytorch pytorch 21919 gh/zou3519/56/head
Added names= arg for some creation functions

- empty, ones, zeros, rand, randn

Test Plan
- new tests [namedtensor ci]

gh-metadata: pytorch pytorch 21919 gh/zou3519/56/head
Added names= arg for some creation functions

- empty, ones, zeros, rand, randn

Test Plan
- new tests [namedtensor ci]

gh-metadata: pytorch pytorch 21919 gh/zou3519/56/head
Added names= arg for some creation functions

- empty, ones, zeros, rand, randn

Test Plan
- new tests [namedtensor ci]

gh-metadata: pytorch pytorch 21919 gh/zou3519/56/head
@zou3519
Copy link
Contributor Author

zou3519 commented Jun 26, 2019

Closing because I want to do this another way.

@zou3519 zou3519 closed this Jun 26, 2019
@facebook-github-bot facebook-github-bot deleted the gh/zou3519/56/head branch October 28, 2019 22:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants