[numpy] `torch.angle`: promote integer inputs to float #49163

kshitij12345 · 2020-12-10T14:53:31Z

BC-Breaking Note:

This PR updates PyTorch's angle operator to be consistent with NumPy's. Previously angle would return zero for all floating point values (including NaN). Now angle returns pi for negative floating point values, zero for non-negative floating point values, and propagates NaNs.

PR Summary:

Reference: #42515

TODO:

Add BC-Breaking Note (Prev all real numbers returned 0 (even nan)) -> Fixed to match the correct behavior of NumPy.

dr-ci · 2020-12-10T16:21:19Z

💊 CI failures summary and remediations

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

1/1 failures possibly* introduced in this PR
- 1/1 non-CircleCI failure(s)

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 to the (internal) Dr. CI Users group.

This comment has been revised 35 times.

codecov · 2020-12-11T08:27:27Z

Codecov Report

Merging #49163 (d95ff19) into master (7b4a766) will increase coverage by 5.48%.
The diff coverage is 93.75%.

@@            Coverage Diff             @@
##           master   #49163      +/-   ##
==========================================
+ Coverage   75.20%   80.69%   +5.48%     
==========================================
  Files        1886     1890       +4     
  Lines      204860   205005     +145     
==========================================
+ Hits       154073   165428   +11355     
+ Misses      50787    39577   -11210

…unary-float-op/angle

test/test_unary_ufuncs.py

mruberry · 2020-12-17T08:14:49Z

aten/src/ATen/native/cpu/UnaryOpsKernel.cpp

@@ -167,7 +167,7 @@ static void abs_kernel(TensorIterator& iter) {
 }

 static void angle_kernel(TensorIterator& iter) {
-  AT_DISPATCH_ALL_TYPES_AND_COMPLEX_AND2(kBFloat16, kHalf, iter.dtype(), "angle_cpu", [&]() {
+  AT_DISPATCH_FLOATING_AND_COMPLEX_TYPES_AND2(kBFloat16, kHalf, iter.dtype(), "angle_cpu", [&]() {


This is not dispatched on common_dtype but dtype?

mruberry · 2020-12-17T08:15:03Z

aten/src/ATen/native/UnaryOps.cpp

 }
 Tensor angle(const Tensor& self) {
-  return unary_op_impl_with_complex_to_float(self, at::angle_out);
+  if (self.is_complex()) {
+    const auto float_type = c10::toValueType(self.scalar_type());


Won't this run afoul of the safe casting logic in TensorIterator?

Oh, sorry, it doesn't because it doesn't call tensor iterator, it calls angle_out.

This is actually similar to what was already there.

pytorch/aten/src/ATen/native/UnaryOps.cpp

Lines 105 to 109 in f5b68e7

if (self.is_complex()) {

const auto float_type = c10::toValueType(self.scalar_type());

Tensor result = at::empty({0}, self.options().dtype(float_type));

return out_impl(result, self);

}

mruberry · 2020-12-17T08:15:59Z

aten/src/ATen/native/cpu/zmath.h

@@ -35,7 +35,10 @@ inline double zabs <c10::complex<double>, double> (c10::complex<double> z) {

 template <typename SCALAR_TYPE, typename VALUE_TYPE=SCALAR_TYPE>


Add a comment explaining this function

I think the name argument/arg is the more appropriate term from complex number perspective. What do you think?
But angle also makes sense as the user facing function is called angle.
About angle_impl, all the functions in zmath.h consistently have _impl suffix.

pytorch/aten/src/ATen/native/cpu/zmath.h

Lines 114 to 163 in 76f7b73

template <typename TYPE>

inline TYPE conj_impl (TYPE z) {

return z; //No-Op

}

template<>

inline c10::complex<float> conj_impl <c10::complex<float>> (c10::complex<float> z) {

return c10::complex<float>(z.real(), -z.imag());

}

template<>

inline c10::complex<double> conj_impl <c10::complex<double>> (c10::complex<double> z) {

return c10::complex<double>(z.real(), -z.imag());

}

template <typename TYPE>

inline TYPE ceil_impl (TYPE z) {

return std::ceil(z);

}

template <>

inline c10::complex<float> ceil_impl (c10::complex<float> z) {

return c10::complex<float>(std::ceil(z.real()), std::ceil(z.imag()));

}

template <>

inline c10::complex<double> ceil_impl (c10::complex<double> z) {

return c10::complex<double>(std::ceil(z.real()), std::ceil(z.imag()));

}

template<typename T>

inline c10::complex<T> sgn_impl (c10::complex<T> z) {

if (z == c10::complex<T>(0, 0)) {

return c10::complex<T>(0, 0);

} else {

return z / zabs(z);

}

}

template <typename TYPE>

inline TYPE floor_impl (TYPE z) {

return std::floor(z);

}

template <>

inline c10::complex<float> floor_impl (c10::complex<float> z) {

return c10::complex<float>(std::floor(z.real()), std::floor(z.imag()));

}

template <>

I guess we're stuck with "_impl", then. If the name is consistent then this PR doesn't need to bother updating it. But a comment explaining it would still be good.

Have added the following comment.

https://github.com/pytorch/pytorch/pull/49163/files#diff-de6356355016a3f8ef55d44f8252b42fd81fca56421b6097427feb1b7a63a5ebR36-R41

mruberry · 2020-12-17T08:16:50Z

aten/src/ATen/native/cuda/UnaryComplexKernels.cu

@@ -11,7 +12,10 @@ namespace at { namespace native {
 // We manually overload angle because std::arg does not work with types other than c10::complex.
 template<typename scalar_t>
 __host__ __device__ static inline scalar_t angle_wrapper(scalar_t v) {


The name "angle_impl" in zmath.h seems odd to me, is there a better name? Maybe one that would fit a descriptive comment better?

mruberry · 2020-12-17T08:17:51Z

aten/src/ATen/native/cuda/UnaryComplexKernels.cu

@@ -20,7 +24,7 @@ __host__ __device__ static inline c10::complex<T> angle_wrapper(c10::complex<T>
 }

 void angle_kernel_cuda(TensorIterator& iter) {
-  AT_DISPATCH_ALL_TYPES_AND_COMPLEX(iter.dtype(), "angle_cuda", [&]() {
+  AT_DISPATCH_FLOATING_AND_COMPLEX_TYPES(iter.common_dtype(), "angle_cuda", [&]() {


Note the dispatch is on the common_dtype here

@kshitij12345 @mruberry why did we change it to common_dtype?

Reference:

#45733 (comment)

torch/testing/_internal/common_methods_invocations.py

mruberry · 2020-12-17T08:21:02Z

torch/testing/_internal/common_methods_invocations.py

@@ -1253,8 +1270,6 @@ def method_tests():
        ('complex', (S, S, S), ((S, S, S),), ''),
        ('abs', (S, S, S), NO_ARGS, '', (True,)),
        ('abs', (), NO_ARGS, 'scalar', (True,)),


Reviewer's note:

There is another test for angle but it also tests abs:

pytorch/test/test_unary_ufuncs.py

Line 538 in 65876d3

def test_abs_angle_complex_to_float(self, device, dtype):

mruberry · 2020-12-17T09:27:14Z

Hey @kshitij12345! It's great to have a more consistent angle, an OpInfo for angle, and a int->float promoting angle. Overall this looks good. It needs a rebase, which should avoid the need for the skip, and I left a couple comments. Some of them are about improving the existing code's clarity.

kshitij12345 · 2020-12-21T12:45:07Z

@mruberry PTAL

mruberry · 2020-12-21T13:02:15Z

torch/_torch_docs.py

@@ -625,6 +625,10 @@ def merge_dicts(*dicts):
 Keyword args:
    {out}

+.. note::  From version 1.8 onwards, the angle function returns `PI` for negative real numbers,


"Starting in PyTorch 1.8, angle returns pi for negative real numbers, zero for non-negative real numbers, and propagates NaNs. Previously the function would return zero for all real numbers and not propagate floating-point NaNs."

mruberry

Looks great! Just one easy docs update and then ping me to merge this.

kshitij12345 · 2020-12-22T09:40:55Z

@mruberry PTAL :)

Failure looks irrelevant.

======================================================================
06:24:49 FAIL [0.096s]: test_embedding_bag_device_cuda_int32_float32 (__main__.TestNNDeviceTypeCUDA)
06:24:49 ----------------------------------------------------------------------
06:24:49 Traceback (most recent call last):
06:24:49   File "/var/lib/jenkins/.local/lib/python3.6/site-packages/torch/testing/_internal/common_utils.py", line 884, in wrapper
06:24:49     method(*args, **kwargs)
06:24:49   File "/var/lib/jenkins/.local/lib/python3.6/site-packages/torch/testing/_internal/common_utils.py", line 884, in wrapper
06:24:49     method(*args, **kwargs)
06:24:49   File "/var/lib/jenkins/.local/lib/python3.6/site-packages/torch/testing/_internal/common_device_type.py", line 273, in instantiated_test
06:24:49     result = test_fn(self, *args)
06:24:49   File "test_nn.py", line 12592, in test_embedding_bag_device
06:24:49     self._test_EmbeddingBag(device, 'mean', False, wdtype=dtypes[1], dtype=dtypes[0])
06:24:49   File "test_nn.py", line 12570, in _test_EmbeddingBag
06:24:49     self._test_EmbeddingBag_vs_Embedding(N, D, B, L, **kwargs)
06:24:49   File "test_nn.py", line 12422, in _test_EmbeddingBag_vs_Embedding
06:24:49     self.assertEqual(output, ref_output, atol=dtype2prec_DONTUSE[wdtype], rtol=0)
06:24:49   File "/var/lib/jenkins/.local/lib/python3.6/site-packages/torch/testing/_internal/common_utils.py", line 1175, in assertEqual
06:24:49     super().assertTrue(result, msg=self._get_assert_msg(msg, debug_msg=debug_msg))
06:24:49 AssertionError: False is not true : Tensors failed to compare as equal!With rtol=0 and atol=1e-05, found 49 element(s) (out of 225) whose difference(s) exceeded the margin of error (including 0 nan comparisons). The greatest difference was 0.315048448741436 (0.06700902432203293 vs. -0.24803942441940308), which occurred at index (0, 49).
06:24:49

https://ci.pytorch.org/jenkins/job/pytorch-builds/job/pytorch-linux-bionic-rocm3.10-py3.6-test2/418/console

mruberry · 2020-12-22T17:37:02Z

Awesome! Would you just resolve the merge conflict?

kshitij12345 · 2020-12-22T18:09:22Z

Done.

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-12-23T05:12:01Z

@mruberry merged this pull request in 461aafe.

anjali411 · 2020-12-28T17:49:19Z

aten/src/ATen/cpu/vec256/vec256_bfloat16.h

+    auto angle_lambda = [](__m256 values) {
+      const auto zero_vec = _mm256_set1_ps(0.f);
+      const auto nan_vec = _mm256_set1_ps(NAN);
+      const auto not_nan_mask = _mm256_cmp_ps(values, values, _CMP_EQ_OQ);


sorry I am not very well versed with the AVX instructions ... what (nan related) information do we get by comparing values to itself?

Since AVX itself does not have a special function for masking nans.
We use the property nan != nan to find the nan values.

kshitij12345 added 2 commits December 10, 2020 08:51

angle: support int -> float promotion

a3321b8

update relevant test and testing mechanism

4d0fac4

facebook-github-bot added the cla signed label Dec 10, 2020

remove dead code path and dispatch generation

4132b4b

pytorchbot added the open source label Dec 10, 2020

kshitij12345 added 3 commits December 10, 2020 12:20

remove redundant test

f794e33

add comment for argument

aa66092

Merge branch 'master' into develop/numpy/unary-float-op/angle

bc3d3a3

kshitij12345 mentioned this pull request Dec 11, 2020

Unary Ufunc tracking issue #42515

Closed

8 tasks

kshitij12345 added 4 commits December 14, 2020 08:08

update dtypes for ROCM

210e492

Merge branch 'develop/numpy/unary-float-op/angle' into develop/numpy/…

399c0e9

…unary-float-op/angle

add bc-breaking note

dd2e91d

add comment

ad5b880

kshitij12345 requested a review from mruberry December 15, 2020 12:55

kshitij12345 marked this pull request as ready for review December 15, 2020 12:55

remove angle entry from method tests

8e69c74

kshitij12345 commented Dec 15, 2020

View reviewed changes

test/test_unary_ufuncs.py Show resolved Hide resolved

kshitij12345 added 2 commits December 16, 2020 22:50

Merge branch 'master' into develop/numpy/unary-float-op/angle

75b90f6

fix stray merge

76f7b73

mruberry reviewed Dec 17, 2020

View reviewed changes

torch/testing/_internal/common_methods_invocations.py Outdated Show resolved Hide resolved

mruberry reviewed Dec 17, 2020

View reviewed changes

Merge branch 'master' into develop/numpy/unary-float-op/angle

d2d9bcb

kshitij12345 added 3 commits December 17, 2020 03:31

use common_dtype in CPU kernel

96b752d

continue is variant is None

da1ce2b

add comment

6b2fdc9

mrshenli added module: numpy Related to numpy support, and also numpy compatibility of our operators triaged This issue has been looked at a team member, and triaged and prioritized into an appropriate module labels Dec 18, 2020

Merge branch 'master' into develop/numpy/unary-float-op/angle

f22b7ee

mruberry added the module: bc-breaking Related to a BC-breaking change label Dec 21, 2020

mruberry reviewed Dec 21, 2020

View reviewed changes

mruberry self-requested a review December 21, 2020 13:03

mruberry approved these changes Dec 21, 2020

View reviewed changes

kshitij12345 added 2 commits December 21, 2020 07:07

address doc comment

df0cb2a

Merge branch 'master' into develop/numpy/unary-float-op/angle

9266941

Merge branch 'master' into develop/numpy/unary-float-op/angle

d95ff19

facebook-github-bot reviewed Dec 22, 2020

View reviewed changes

facebook-github-bot closed this in 461aafe Dec 23, 2020

facebook-github-bot added the Merged label Dec 23, 2020

kshitij12345 deleted the develop/numpy/unary-float-op/angle branch December 23, 2020 05:14

kshitij12345 mentioned this pull request Dec 23, 2020

torch.angle is divergent from numpy.angle #41240

Closed

anjali411 reviewed Dec 28, 2020

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[numpy] `torch.angle`: promote integer inputs to float #49163

[numpy] `torch.angle`: promote integer inputs to float #49163

kshitij12345 commented Dec 10, 2020 •

edited by mruberry

dr-ci bot commented Dec 10, 2020 •

edited by facebook-github-bot

codecov bot commented Dec 11, 2020 •

edited

mruberry Dec 17, 2020

mruberry Dec 17, 2020

mruberry Dec 17, 2020

kshitij12345 Dec 17, 2020

mruberry Dec 17, 2020

kshitij12345 Dec 17, 2020

mruberry Dec 17, 2020

kshitij12345 Dec 17, 2020

mruberry Dec 17, 2020

mruberry Dec 17, 2020

anjali411 Dec 28, 2020

kshitij12345 Dec 28, 2020

mruberry Dec 17, 2020

mruberry commented Dec 17, 2020

kshitij12345 commented Dec 21, 2020

mruberry Dec 21, 2020

mruberry left a comment

kshitij12345 commented Dec 22, 2020

mruberry commented Dec 22, 2020

kshitij12345 commented Dec 22, 2020

facebook-github-bot left a comment

facebook-github-bot commented Dec 23, 2020

anjali411 Dec 28, 2020

kshitij12345 Dec 28, 2020

	if (self.is_complex()) {
	const auto float_type = c10::toValueType(self.scalar_type());
	Tensor result = at::empty({0}, self.options().dtype(float_type));
	return out_impl(result, self);
	}

		@@ -35,7 +35,10 @@ inline double zabs <c10::complex<double>, double> (c10::complex<double> z) {

		template <typename SCALAR_TYPE, typename VALUE_TYPE=SCALAR_TYPE>

	template <typename TYPE>
	inline TYPE conj_impl (TYPE z) {
	return z; //No-Op
	}

	template<>
	inline c10::complex<float> conj_impl <c10::complex<float>> (c10::complex<float> z) {
	return c10::complex<float>(z.real(), -z.imag());
	}

	template<>
	inline c10::complex<double> conj_impl <c10::complex<double>> (c10::complex<double> z) {
	return c10::complex<double>(z.real(), -z.imag());
	}

	template <typename TYPE>
	inline TYPE ceil_impl (TYPE z) {
	return std::ceil(z);
	}

	template <>
	inline c10::complex<float> ceil_impl (c10::complex<float> z) {
	return c10::complex<float>(std::ceil(z.real()), std::ceil(z.imag()));
	}

	template <>
	inline c10::complex<double> ceil_impl (c10::complex<double> z) {
	return c10::complex<double>(std::ceil(z.real()), std::ceil(z.imag()));
	}

	template<typename T>
	inline c10::complex<T> sgn_impl (c10::complex<T> z) {
	if (z == c10::complex<T>(0, 0)) {
	return c10::complex<T>(0, 0);
	} else {
	return z / zabs(z);
	}
	}

	template <typename TYPE>
	inline TYPE floor_impl (TYPE z) {
	return std::floor(z);
	}

	template <>
	inline c10::complex<float> floor_impl (c10::complex<float> z) {
	return c10::complex<float>(std::floor(z.real()), std::floor(z.imag()));
	}

	template <>

[numpy] torch.angle: promote integer inputs to float #49163

[numpy] torch.angle: promote integer inputs to float #49163

Conversation

kshitij12345 commented Dec 10, 2020 • edited by mruberry

dr-ci bot commented Dec 10, 2020 • edited by facebook-github-bot

💊 CI failures summary and remediations

codecov bot commented Dec 11, 2020 • edited

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mruberry commented Dec 17, 2020

kshitij12345 commented Dec 21, 2020

Choose a reason for hiding this comment

mruberry left a comment

Choose a reason for hiding this comment

kshitij12345 commented Dec 22, 2020

mruberry commented Dec 22, 2020

kshitij12345 commented Dec 22, 2020

facebook-github-bot left a comment

Choose a reason for hiding this comment

facebook-github-bot commented Dec 23, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

[numpy] `torch.angle`: promote integer inputs to float #49163

[numpy] `torch.angle`: promote integer inputs to float #49163

kshitij12345 commented Dec 10, 2020 •

edited by mruberry

dr-ci bot commented Dec 10, 2020 •

edited by facebook-github-bot

codecov bot commented Dec 11, 2020 •

edited