An implementation of torch.tile as requested in pytorch/pytorch#38349 by arbfay · Pull Request #47974 · pytorch/pytorch

arbfay · 2020-11-14T22:10:23Z

The approach is to simply reuse torch.repeat but adding one more functionality to tile, which is to prepend 1's to reps arrays if there are more dimensions to the tensors than the reps given in input. Thus for a tensor of shape (64, 3, 24, 24) and reps of (2, 2) will become (1, 1, 2, 2), which is what NumPy does.

I've encountered some instability with the test on my end, where I could get a random failure of the test (due to, sometimes, random value of self.dim(), and sometimes, segfaults). I'd appreciate any feedback on the test or an explanation for this instability so I can this.

facebook-github-bot · 2020-11-14T22:10:47Z

Hi @arbfay!

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file.

In order for us to review and merge your code, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA.

If you have received this in error or have any questions, please contact us at cla@fb.com. Thanks!

dr-ci · 2020-11-15T00:02:39Z

💊 CI failures summary and remediations

As of commit e253f12 (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 68 times.

zou3519 · 2020-11-16T16:35:35Z

cc @mruberry for numpy compatibility - could you take a look at this please?

mruberry

Hi @arbfay, thanks for this contribution!

Overall this PR looks pretty good. I'm not totally sure why you're getting the error you are. It may be as simple as calling repeat instead of at::repeat. You might have to look at the intermediate values and debug the issue, thought.

I've left some comments that are mostly style/docs suggestions. The tests also need to be more thorough to account for more possible cases.

Looking forward to reviewing the follow-up!

arbfay · 2020-11-18T08:51:06Z

Hi @mruberry,

Thanks for this review! I expected lots of comments for my first PR to PyTorch, and I got served 😄
For the failures, I've increased stability as much as I could with the last two commits. Hopefully, it will be enough now for all the pipelines. (Okay I will say it: it works perfectly in my environment)

Waiting now for your next review

mruberry · 2020-11-18T10:39:07Z

This failure on ROCm is real:

10:24:18 ======================================================================
10:24:18 ERROR [0.051s]: test_tile_cuda (__main__.TestTorchDeviceTypeCUDA)
10:24:18 ----------------------------------------------------------------------
10:24:18 Traceback (most recent call last):
10:24:18   File "/var/lib/jenkins/.local/lib/python3.6/site-packages/torch/testing/_internal/common_utils.py", line 834, in wrapper
10:24:18     method(*args, **kwargs)
10:24:18   File "/var/lib/jenkins/.local/lib/python3.6/site-packages/torch/testing/_internal/common_device_type.py", line 272, in instantiated_test
10:24:18     result = test_fn(self, *args)
10:24:18   File "/var/lib/jenkins/.local/lib/python3.6/site-packages/torch/testing/_internal/common_device_type.py", line 667, in only_fn
10:24:18     return fn(self, device, *args, **kwargs)
10:24:18   File "test_torch.py", line 6071, in test_tile
10:24:18     expected = np.tile(t.numpy(), dims)
10:24:18 TypeError: can't convert cuda:0 device type tensor to numpy. Use Tensor.cpu() to copy the tensor to host memory first.
10:24:18 
10:24:20 ----------------------------------------------------------------------

You can always use the pattern t.cpu().numpy(). This will convert CUDA tensors to CPU tensors (and then convert those to NumPy) and just directly convert CPU tensors to NumPy (the .cpu() will be a no-op for them).

Would you correct this test and I'll take a look after we get an updated test signal?

arbfay · 2020-11-18T16:59:59Z

@mruberry I think it's all ok with the tests (the one failing is because the pipeline is broken I think)

mruberry

Hey @arbfay!

Just a couple minor implementation issues and a small doc extension + test extension needed. Otherwise this PR looks good.

For the tests, this is currently missing autograd tests. That's understandable since we don't do a good job explaining how to add them.

You can automatically generate autograd tests by adding an entry to

pytorch/torch/testing/_internal/common_methods_invocations.py

Line 574 in 68a3a3f

def method_tests():

#46041 recently did this, too, so that might be a useful reference. Let me know if you have any questions.

arbfay · 2020-11-19T16:09:43Z

Thanks @mruberry !

I've been struggling a bit with the autograd tests. I've added more doc as asked, and used self.repeat instead of at::native::repeat for the return statements.

mruberry · 2020-11-20T10:49:41Z

I hope you don't mind but I tweaked the docs slight and tried to fix the common_methods() tests. If my fix for the tests didn't work properly then we can remove them for now to unblock this PR. Let's just wait for the CI to run.

…ch.Size call in common_methods_invocations

arbfay · 2020-11-20T11:22:47Z

Thanks for your commits @mruberry, they are more than welcome!

I've tried with your modification in common_methods and only one test is still a problem (no_reps_dims, I pushed a commit to remove it). This is great, I've been struggling to understand what was causing those failed assertions.

Let's wait for the pipelines.

codecov · 2020-11-20T19:34:23Z

Codecov Report

Merging #47974 (e253f12) into master (6da26fe) will decrease coverage by 0.00%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master   #47974      +/-   ##
==========================================
- Coverage   81.15%   81.14%   -0.01%     
==========================================
  Files        1838     1838              
  Lines      198653   198663      +10     
==========================================
- Hits       161214   161211       -3     
- Misses      37439    37452      +13

arbfay · 2020-11-20T21:27:45Z

@mruberry is the PR good to go?

mruberry

Tests look good. Nice work, @arbfay!

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-11-25T03:13:01Z

@mruberry merged this pull request in 2e0a8b7.

mruberry · 2020-11-25T10:48:16Z

Congrats @arbfay! Thanks for implementing torch.tile!

arbfay · 2020-11-25T11:25:07Z

Thanks @mruberry, and thanks for the help! Been a nice ride, and learned a lot!

…h#47974) Summary: The approach is to simply reuse `torch.repeat` but adding one more functionality to tile, which is to prepend 1's to reps arrays if there are more dimensions to the tensors than the reps given in input. Thus for a tensor of shape (64, 3, 24, 24) and reps of (2, 2) will become (1, 1, 2, 2), which is what NumPy does. I've encountered some instability with the test on my end, where I could get a random failure of the test (due to, sometimes, random value of `self.dim()`, and sometimes, segfaults). I'd appreciate any feedback on the test or an explanation for this instability so I can this. Pull Request resolved: pytorch#47974 Reviewed By: ngimel Differential Revision: D25148963 Pulled By: mruberry fbshipit-source-id: bf63b72c6fe3d3998a682822e669666f7cc97c58

pytorchbot added the open source label Nov 14, 2020

facebook-github-bot added the cla signed label Nov 14, 2020

zou3519 requested a review from mruberry November 16, 2020 16:35

zou3519 added the triaged This issue has been looked at a team member, and triaged and prioritized into an appropriate module label Nov 16, 2020