Add complex support for torch.mean [CUDA] by RockingJavaBean · Pull Request #47048 · pytorch/pytorch

RockingJavaBean · 2020-10-29T11:25:50Z

Fixes #46982

dr-ci · 2020-10-29T12:23:19Z

💊 CI failures summary and remediations

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

1/1 failures introduced in this PR

🕵️ 1 new failure recognized by patterns

The following CI failures do not appear to be due to upstream breakages:

pytorch_vulkan_linux_bionic_py3_6_clang9_build (1/1)

Step: "Build" (full log | diagnosis details | 🔁 rerun)

Nov 06 16:53:16 sccache: error: couldn't connect to server

Nov 06 16:53:16 +++ eval 'extract_trap_cmd ' 
Nov 06 16:53:16 ++++ extract_trap_cmd 
Nov 06 16:53:16 ++++ printf '%s\n' '' 
Nov 06 16:53:16 +++ printf '%s\n' cleanup 
Nov 06 16:53:16 ++ trap -- ' 
Nov 06 16:53:16 cleanup' EXIT 
Nov 06 16:53:16 ++ [[ pytorch-vulkan-linux-bionic-py3.6-clang9-build != *pytorch-win-* ]] 
Nov 06 16:53:16 ++ which sccache 
Nov 06 16:53:16 ++ sccache --stop-server 
Nov 06 16:53:16 Stopping sccache server... 
Nov 06 16:53:16 sccache: error: couldn't connect to server 
Nov 06 16:53:16 sccache: caused by: Connection refused (os error 111) 
Nov 06 16:53:16 ++ true 
Nov 06 16:53:16 ++ rm /var/lib/jenkins/sccache_error.log 
Nov 06 16:53:16 rm: cannot remove '/var/lib/jenkins/sccache_error.log': No such file or directory 
Nov 06 16:53:16 ++ true 
Nov 06 16:53:16 ++ [[ pytorch-vulkan-linux-bionic-py3.6-clang9-build == *rocm* ]] 
Nov 06 16:53:16 ++ SCCACHE_ERROR_LOG=/var/lib/jenkins/sccache_error.log 
Nov 06 16:53:16 ++ SCCACHE_IDLE_TIMEOUT=1200 
Nov 06 16:53:16 ++ RUST_LOG=sccache::server=error 
Nov 06 16:53:16 ++ sccache --start-server

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 33 times.

codecov · 2020-11-02T07:01:59Z

Codecov Report

Merging #47048 into master will increase coverage by 17.31%.
The diff coverage is n/a.

@@             Coverage Diff             @@
##           master   #47048       +/-   ##
===========================================
+ Coverage   35.95%   53.27%   +17.31%     
===========================================
  Files         438     2747     +2309     
  Lines       55454   254304   +198850     
===========================================
+ Hits        19939   135476   +115537     
- Misses      35515   118828    +83313

RockingJavaBean · 2020-11-02T12:51:00Z

@anjali411 Thanks so much for reviewing this PR.
The latest PR leverages the explicit template specialization to handle factor with complex tensors, instead of casting factor to scalar_t.
The CI checks all passed, please kindly help review.

anjali411 · 2020-11-03T16:24:29Z

@@ -36,26 +36,40 @@ static void std_var_kernel_cuda(TensorIterator& iter, bool unbiased, bool take_s

 template <typename scalar_t, typename acc_t=scalar_t, typename out_t=scalar_t>


setting acc_t = typename c10::scalar_value_type<scalar_t>::type should resolve the issue here

c10::scalar_value_type<scalar_t>::type returns scalar_t for all non-complex dtypes and returns T for c10::complex<T>.

Thanks so much for the tip. The latest code now manipulates c10::scalar_value_type<acc_t>::type to get the type of the factor, the overload functions for complex numbers are not needed.

anjali411 · 2020-11-03T16:28:08Z

@anjali411 Thanks so much for reviewing this PR.
The latest PR leverages the explicit template specialization to handle factor with complex tensors, instead of casting factor to scalar_t.

Hi @RockingJavaBean I think we shouldn't need to define overload functions for complex types, after the change I suggested in my comment. But this PR looks great overall, and should be ready to merge after that change!

…h_mean_complex

facebook-github-bot

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

anjali411 · 2020-11-04T17:06:15Z

                lambda n, d: n.mean(d),
-                use_integral=False)
+                use_integral=False,
+                use_complex=True)


This test doesn't run on CUDA. can you please extend the test for mean in tensor_op_tests to also test complex dtypes?

Thanks for pointing this out, the tests for complex dtypes are added to tensor_op_tests.

anjali411

let's update the CUDA test for mean to test complex dtypes as well

…h_mean_complex

RockingJavaBean · 2020-11-05T12:48:46Z

@anjali411 I'm really grateful for your tip on c10::scalar_value_type, and the tests for complex inputs of torch.mean function are updated.
Please kindly help review.

anjali411

lgtm and the windows test failure is an upstream test failure

facebook-github-bot

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

RockingJavaBean · 2020-11-05T16:11:34Z

@anjali411 thanks so much for reviewing this PR, the CUDA tests for torch.mean passed before the upstream failure CI check terminated according to the circleci log.

test_mean_cuda_bfloat16 (__main__.TestTensorDeviceOpsCUDA) ... ok (0.005s)
test_mean_cuda_complex128 (__main__.TestTensorDeviceOpsCUDA) ... ok (0.004s)
test_mean_cuda_complex64 (__main__.TestTensorDeviceOpsCUDA) ... ok (0.004s)
test_mean_cuda_float16 (__main__.TestTensorDeviceOpsCUDA) ... ok (0.004s)
test_mean_cuda_float32 (__main__.TestTensorDeviceOpsCUDA) ... ok (0.003s)
test_mean_cuda_float64 (__main__.TestTensorDeviceOpsCUDA) ... ok (0.004s)
test_mean_dim_cuda_bfloat16 (__main__.TestTensorDeviceOpsCUDA) ... ok (0.004s)
test_mean_dim_cuda_complex128 (__main__.TestTensorDeviceOpsCUDA) ... ok (0.005s)
test_mean_dim_cuda_complex64 (__main__.TestTensorDeviceOpsCUDA) ... ok (0.004s)
test_mean_dim_cuda_float16 (__main__.TestTensorDeviceOpsCUDA) ... ok (0.004s)
test_mean_dim_cuda_float32 (__main__.TestTensorDeviceOpsCUDA) ... ok (0.004s)
test_mean_dim_cuda_float64 (__main__.TestTensorDeviceOpsCUDA) ... ok (0.003s)
test_mean_neg_dim_cuda_bfloat16 (__main__.TestTensorDeviceOpsCUDA) ... ok (0.005s)
test_mean_neg_dim_cuda_complex128 (__main__.TestTensorDeviceOpsCUDA) ... ok (0.004s)
test_mean_neg_dim_cuda_complex64 (__main__.TestTensorDeviceOpsCUDA) ... ok (0.004s)
test_mean_neg_dim_cuda_float16 (__main__.TestTensorDeviceOpsCUDA) ... ok (0.004s)
test_mean_neg_dim_cuda_float32 (__main__.TestTensorDeviceOpsCUDA) ... ok (0.004s)
test_mean_neg_dim_cuda_float64 (__main__.TestTensorDeviceOpsCUDA) ... ok (0.003s

anjali411 · 2020-11-06T16:33:41Z

@RockingJavaBean can you please rebase?

…h_mean_complex

RockingJavaBean · 2020-11-06T16:46:07Z

@anjali411 thank you so much for the kind reminder, I just rebased this PR with the latest code.

facebook-github-bot

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

facebook-github-bot · 2020-11-07T07:10:40Z

@anjali411 merged this pull request in f90da88.

Summary: Fixes pytorch#46982 Pull Request resolved: pytorch#47048 Reviewed By: heitorschueroff Differential Revision: D24729895 Pulled By: anjali411 fbshipit-source-id: 8e948480eb87c37de810207edf909375c0380772

pytorchbot added the open source label Oct 29, 2020

anjali411 reviewed Oct 29, 2020

View reviewed changes

Comment thread aten/src/ATen/native/cuda/ReduceMomentKernel.cu Outdated

anjali411 added the module: complex Related to complex number support in PyTorch label Oct 30, 2020

RockingJavaBean changed the title ~~[WIP] Enable complex numbers for torch.mean of CUDA path~~ Enable complex numbers for torch.mean of CUDA path Oct 30, 2020

ngimel added the triaged This issue has been looked at a team member, and triaged and prioritized into an appropriate module label Oct 30, 2020

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

Enable complex numbers for torch.mean of CUDA path

b0de1be

RockingJavaBean force-pushed the torch_mean_complex branch from d618111 to b0de1be Compare November 2, 2020 07:01

RockingJavaBean requested a review from anjali411 November 3, 2020 13:15

anjali411 reviewed Nov 3, 2020

View reviewed changes

RockingJavaBean added 2 commits November 4, 2020 19:33

use c10::scalar_value_type to get factor type

5154e70

Merge branch 'master' of https://github.com/pytorch/pytorch into torc…

82d43fe

…h_mean_complex

facebook-github-bot reviewed Nov 4, 2020

View reviewed changes

anjali411 approved these changes Nov 4, 2020

View reviewed changes

anjali411 reviewed Nov 4, 2020

View reviewed changes

RockingJavaBean added 2 commits November 5, 2020 11:42

expand the complex numbers test for mean to cuda

25615ba

Merge branch 'master' of https://github.com/pytorch/pytorch into torc…

79e93e0

…h_mean_complex

RockingJavaBean requested a review from anjali411 November 5, 2020 12:48

anjali411 approved these changes Nov 5, 2020

View reviewed changes

facebook-github-bot reviewed Nov 5, 2020

View reviewed changes

anjali411 changed the title ~~Enable complex numbers for torch.mean of CUDA path~~ Add complex support for torch.mean [CUDA] Nov 5, 2020

anjali411 mentioned this pull request Nov 5, 2020

Complex Numbers Support #33152

Closed

Merge branch 'master' of https://github.com/pytorch/pytorch into torc…

de87d22

…h_mean_complex

facebook-github-bot reviewed Nov 6, 2020

View reviewed changes

facebook-github-bot closed this in f90da88 Nov 7, 2020

facebook-github-bot added the Merged label Nov 7, 2020

RockingJavaBean deleted the torch_mean_complex branch November 10, 2020 05:46

		@@ -36,26 +36,40 @@ static void std_var_kernel_cuda(TensorIterator& iter, bool unbiased, bool take_s

		template <typename scalar_t, typename acc_t=scalar_t, typename out_t=scalar_t>

Conversation

RockingJavaBean commented Oct 29, 2020

Uh oh!

dr-ci Bot commented Oct 29, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

💊 CI failures summary and remediations

🕵️ 1 new failure recognized by patterns

pytorch_vulkan_linux_bionic_py3_6_clang9_build (1/1)

Uh oh!

Uh oh!

codecov Bot commented Nov 2, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

RockingJavaBean commented Nov 2, 2020

Uh oh!

anjali411 Nov 3, 2020

Choose a reason for hiding this comment

Uh oh!

RockingJavaBean Nov 5, 2020

Choose a reason for hiding this comment

Uh oh!

anjali411 commented Nov 3, 2020

Uh oh!

facebook-github-bot left a comment

Choose a reason for hiding this comment

Uh oh!

anjali411 Nov 4, 2020

Choose a reason for hiding this comment

Uh oh!

RockingJavaBean Nov 5, 2020

Choose a reason for hiding this comment

Uh oh!

anjali411 left a comment

Choose a reason for hiding this comment

Uh oh!

RockingJavaBean commented Nov 5, 2020

Uh oh!

anjali411 left a comment

Choose a reason for hiding this comment

Uh oh!

facebook-github-bot left a comment

Choose a reason for hiding this comment

Uh oh!

RockingJavaBean commented Nov 5, 2020

Uh oh!

anjali411 commented Nov 6, 2020

Uh oh!

RockingJavaBean commented Nov 6, 2020

Uh oh!

facebook-github-bot left a comment

Choose a reason for hiding this comment

Uh oh!

facebook-github-bot commented Nov 7, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

dr-ci Bot commented Oct 29, 2020 •

edited

Loading

codecov Bot commented Nov 2, 2020 •

edited

Loading