Add `torch.atleast_{1d/2d/3d}` #41317

kshitij12345 · 2020-07-12T06:42:32Z

#38349

TODO:

Docs
Tests

dr-ci · 2020-07-12T14:20:34Z

💊 CI failures summary and remediations

As of commit c6dd6de (more details on the Dr. CI page):

💚 💚 Looks good so far! There are no failures yet. 💚 💚

This comment was automatically generated by Dr. CI (expand for details).

Follow this link to opt-out of these comments for your Pull Requests.

Please report bugs/suggestions on the GitHub issue tracker or post in the (internal) Dr. CI Users group.

See how this bot performed.

This comment has been revised 59 times.

kshitij12345 · 2020-07-13T13:20:27Z

@mruberry

Please review:)

aten/src/ATen/native/TensorTransformations.cpp

torch/functional.py

mruberry · 2020-07-14T08:15:19Z

torch/functional.py

    else:
        raise ValueError("{} is not a valid value for compute_mode".format(compute_mode))

+def atleast_1d(*tensors):


Do you really need to document this here? Seems like it could go in _torch_docs.py?

Tried moving them to _torch_docs but on importing got

E TypeError: don't know how to add docstring to type 'function'

I guess keeping the documentation in functional.py is in-spirit with the functions already present in that file.

Hm. You should definitely be able to add them to _torch_docs since these are new docstrings!

Docstrings should really be in _torch_docs.py. Having them elsewhere is a pain. I don't think you need to define these functions in that file at all.

I agree that having the else is not intuitive to find ( faced that while trying to find meshgrid) but I think this is where it will have to stay here.

I don't think you need to define these functions in that file at all.

The reason the function is in functional.py is so that it can support the torch.atleast_1d(x,y,z) signature similar to numpy which does. The signatures in native_functions.yaml are wrapped to support the above signature.

- func: atleast_1d(Tensor self) -> Tensor use_c10_dispatcher: full variants: function - func: atleast_1d.Sequence(Tensor[] tensors) -> Tensor[] use_c10_dispatcher: full

I tried this

>>> torch._C._add_docstr(torch.atleast_1d, "Test Doc") Traceback (most recent call last): File "<stdin>", line 1, in <module> TypeError: don't know how to add docstring to type 'function' >>> torch._C._add_docstr(torch.gt, "Test Doc") Traceback (most recent call last): File "<stdin>", line 1, in <module> RuntimeError: function 'gt' already has a docstring

Also I looked at Module.cpp which has the code for _add_docstr.

pytorch/torch/csrc/Module.cpp

Line 210 in c20426f

PyObject *THPModule_addDocStr(PyObject *_unused, PyObject *args)

the type of the new atleast_1d function falls in the following error branch.

pytorch/torch/csrc/Module.cpp

Line 260 in c20426f

return PyErr_Format(PyExc_TypeError,

Also checking the type of few functions, the type of the functions added in functional.py is different from the ones which are present in _torch_docs.py.

>>> torch.atleast_1d # functional.py <function atleast_1d at 0x7f4c3336c510> >>> torch.meshgrid # functional.py <function meshgrid at 0x7f4c3329a950> >>> torch.add # _torch_docs <built-in method add of type object at 0x7f4c5c45f000> >>> torch.combinations # _torch_docs <built-in method combinations of type object at 0x7f4c5c45f000>

mruberry

Overall this looks great, thanks for the PR, @kshitij12345!

I made a few code cleanup comments and have some suggestions for the docs. I hope we can put them in the _torch_docs.py file, too, and get rid of that JIT-specific code.

I really like your test, by the way.

kshitij12345 · 2020-07-14T13:29:45Z

get rid of that JIT-specific code

Tried removing it 9dce44a
but with that we get

Jul 14 03:24:29 ======================================================================
Jul 14 03:24:29 ERROR: test_torch_functional_atleast_1d (__main__.TestTorchFunctionOverride)
Jul 14 03:24:29 ----------------------------------------------------------------------
Jul 14 03:24:29 Traceback (most recent call last):
Jul 14 03:24:29   File "test_overrides.py", line 475, in test
Jul 14 03:24:29     self.assertEqual(func(*func_args), -1)
Jul 14 03:24:29   File "/Users/distiller/workspace/miniconda3/lib/python3.7/site-packages/torch/functional.py", line 1013, in atleast_1d
Jul 14 03:24:29     return _VF.atleast_1d(tensors)
Jul 14 03:24:29 TypeError: expected Tensor as element 0 in argument 0, but got TensorLike

https://dr.pytorch.org/api/view-log-full?build_id=122698430

On a bit digging, looks like the JIT branch is needed to handle __torch_function_ support.

pytorch/torch/_overrides.py

Line 734 in c20426f

def handle_torch_function(

So will add that branch again.

mruberry · 2020-07-16T05:45:08Z

get rid of that JIT-specific code

Tried removing it 9dce44a
but with that we get
Jul 14 03:24:29 ======================================================================
Jul 14 03:24:29 ERROR: test_torch_functional_atleast_1d (__main__.TestTorchFunctionOverride)
Jul 14 03:24:29 ----------------------------------------------------------------------
Jul 14 03:24:29 Traceback (most recent call last):
Jul 14 03:24:29   File "test_overrides.py", line 475, in test
Jul 14 03:24:29     self.assertEqual(func(*func_args), -1)
Jul 14 03:24:29   File "/Users/distiller/workspace/miniconda3/lib/python3.7/site-packages/torch/functional.py", line 1013, in atleast_1d
Jul 14 03:24:29     return _VF.atleast_1d(tensors)
Jul 14 03:24:29 TypeError: expected Tensor as element 0 in argument 0, but got TensorLike
https://dr.pytorch.org/api/view-log-full?build_id=122698430

On a bit digging, looks like the JIT branch is needed to handle __torch_function_ support.

pytorch/torch/_overrides.py

Line 734 in c20426f

def handle_torch_function(

So will add that branch again.

How can that be, though? Many functions, like the recently added lcm and gcd #40651, don't have the same issue and they support overriding. What's the difference between functions like that and these?

kshitij12345 · 2020-07-16T06:03:43Z

The difference is that those functions rely on bindings generated from native_functions.yaml.

For this one, we generate

pytorch/aten/src/ATen/native/native_functions.yaml

Lines 495 to 500 in 2700938

    
           - func: atleast_1d(Tensor self) -> Tensor 
        
             use_c10_dispatcher: full 
        
             variants: function 
        
           - func: atleast_1d.Sequence(Tensor[] tensors) -> Tensor[] 
        
             use_c10_dispatcher: full

So this allows,

x = torch.randn(3)
torch.atleast_1d(x)
torch.atleast_1d((x,x))

torch.atleast_1d(x,x,x) # Will fail. However this is supported by numpy.

So to support torch.atleast_1d(x,x,x), we wrap it in functional.py to support *args type of arguments and wrap them in list to pass it to the correct variant.

pytorch/torch/functional.py

Line 987 in 2700938

def atleast_1d(*tensors):

However with this, like mentioned above the type of the wrapped function is different from the type which of the ones generated from native_functions.yaml and thus _add_docstr does not support them.

I guess that is why all the functions which support *args are wrapped in functional.py like meshgrid, block_diag, broadcast_tensors and all of them have documentation in functional.py itself.

mruberry · 2020-07-16T06:13:21Z

The difference is that those functions rely on bindings generated from native_functions.yaml.

For this one, we generate

pytorch/aten/src/ATen/native/native_functions.yaml

Lines 495 to 500 in 2700938

- func: atleast_1d(Tensor self) -> Tensor

use_c10_dispatcher: full

variants: function

- func: atleast_1d.Sequence(Tensor[] tensors) -> Tensor[]

use_c10_dispatcher: full

So this allows,
x = torch.randn(3)
torch.atleast_1d(x)
torch.atleast_1d((x,x))

torch.atleast_1d(x,x,x) # Will fail. However this is supported by numpy.
So to support torch.atleast_1d(x,x,x), we wrap it in functional.py to support *args type of arguments and wrap them in list to pass it to the correct variant.

pytorch/torch/functional.py

Line 987 in 2700938

def atleast_1d(*tensors):

However with this, like mentioned above the type of the wrapped function is different from the type which of the ones generated from native_functions.yaml and thus _add_docstr does not support them.

I guess that is why all the functions which support *args are wrapped in functional.py like meshgrid, block_diag, broadcast_tensors and all of them have documentation in functional.py itself.

Interesting! So we don't support passing arbitrarily long lists in native_functions.yaml. That makes a lot of sense. Thanks for taking the time and explaining that to me, @kshitij12345! Sounds like you're doing the right thing.

Let me know when you're ready for review again.

fyi @bhosmer

kshitij12345 · 2020-07-16T06:14:57Z

I believe the PR is ready for review. :)

mruberry · 2020-07-16T06:39:45Z

torch/functional.py


+def atleast_1d(*tensors):
+    r"""
+    atleast_1d(input) -> Tensor


atleast_1d(input) -> Tensor or list of Tensors?

Will remove this line as we already have

pytorch/torch/functional.py

Lines 993 to 994 in 2700938

Args:

input (Tensor or list of Tensors)

torch/functional.py

mruberry · 2020-07-16T06:42:44Z

torch/functional.py

+        tensor(0.5000)
+        >>> torch.atleast_3d(x)
+        tensor([[[0.5000]]])
+        >>> x = torch.randn(1,1)


How about a 2x2 example?

torch/functional.py

mruberry · 2020-07-16T06:44:23Z

Still looks very, very good. Made a couple notes and asked a new question about the return type (tuple vs list). Let me know your thoughts!

* update example * fix typo * add return type * remove incorrect signature of function

kshitij12345 · 2020-07-16T13:52:23Z

@mruberry

Had forgotten to add autograd test. Have added it now.
Please review.
Thanks!

mruberry

Awesome! Great work as always, @kshitij12345!

facebook-github-bot

@mruberry has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

facebook-github-bot · 2020-07-17T19:37:49Z

@mruberry merged this pull request in 71fdf74.

mruberry · 2020-07-17T19:45:05Z

Merged! Congrats, @kshitij12345!

kshitij12345 added 3 commits July 12, 2020 12:05

initial implementation atleast_1*

a13d49c

add test

7d112f1

add override signature

bcdd056

pytorchbot added the open source label Jul 12, 2020

kshitij12345 added 6 commits July 12, 2020 14:54

add aten interned strings

27fd612

add docs

cd5edd9

add sequence variant

43612d4

add sequence test

4903fc2

update overrides for sequence variant

9d9ac49

update docs

d54fd02

kshitij12345 added 4 commits July 12, 2020 20:13

preallocate result vector

1532e8d

redirect through torch fucntional and update docs

d738f23

retrigger CI

ea01031

stop autogeneration of type stubs for atleast_*d

a5f60fa

ailzhang requested a review from mruberry July 13, 2020 15:50

ailzhang added the triaged This issue has been looked at a team member, and triaged and prioritized into an appropriate module label Jul 13, 2020