Stop using c10::scalar_to_tensor in float_power. by gchanan · Pull Request #50105 · pytorch/pytorch

gchanan · 2021-01-05T19:38:20Z

Stack from ghstack:

Stop moving scalars to GPU for one computation in leaky_rrelu_backward. #50115 Stop moving scalars to GPU for one computation in leaky_rrelu_backward.
Stop using an unnecessary scalar_to_tensor(..., device) call. #50114 Stop using an unnecessary scalar_to_tensor(..., device) call.
Move scalar_to_tensor_default_dtype out of ScalarOps.h because it's only useful for torch.where. #50111 Move scalar_to_tensor_default_dtype out of ScalarOps.h because it's only useful for torch.where.
Stop using c10::scalar_to_tensor in float_power. #50105 Stop using c10::scalar_to_tensor in float_power.

There should be no functional change here.

A couple of reasons here:

This function is generally an anti-pattern (c10::scalar_to_tensor(...) uses should be audited for performance and type promotion impact #49758) and it is good to minimize its usage in the code base.
pow itself has a fair amount of smarts like not broadcasting scalar/tensor combinations and we should defer to it.

Differential Revision: D25786172

There should be no functional change here. A couple of reasons here: 1) This function is generally an anti-pattern (#49758) and it is good to minimize its usage in the code base. 2) pow itself has a fair amount of smarts like not broadcasting scalar/tensor combinations and we should defer to it.

facebook-github-bot · 2021-01-05T19:38:29Z

💊 CI failures summary and remediations

As of commit 6ee0044 (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 54 times.

There should be no functional change here. A couple of reasons here: 1) This function is generally an anti-pattern (#49758) and it is good to minimize its usage in the code base. 2) pow itself has a fair amount of smarts like not broadcasting scalar/tensor combinations and we should defer to it. ghstack-source-id: 3222bdf Pull Request resolved: #50105

There should be no functional change here. A couple of reasons here: 1) This function is generally an anti-pattern (#49758) and it is good to minimize its usage in the code base. 2) pow itself has a fair amount of smarts like not broadcasting scalar/tensor combinations and we should defer to it. Differential Revision: [D25786172](https://our.internmc.facebook.com/intern/diff/D25786172)

mruberry · 2021-01-06T14:33:11Z

 Tensor& float_power_out(Tensor& result, const Tensor& base, Scalar exp) {
-  return at::float_power_out(result, base, c10::scalar_to_tensor(exp, base.device()));
+  auto dtype = (at::isComplexType(base.scalar_type()) || exp.isComplex()) ? at::kComplexDouble : at::kDouble;
+  exp = dtype == at::kComplexDouble ? exp.toComplexDouble() : exp.toDouble();


Parens after the "=" might clarify this expression

mruberry · 2021-01-06T14:38:03Z

-  return base.float_power_(c10::scalar_to_tensor(exp, base.device()));
+  auto dtype = (at::isComplexType(base.scalar_type()) || exp.isComplex()) ? at::kComplexDouble : at::kDouble;
+  TORCH_CHECK(base.scalar_type() == dtype,
+              "self tensor type ", base.scalar_type(), "is not the desired type ", dtype);


We're inconsistent in our checks (note: we should pick a style an be consistent) but typically want to mention the operation and use the name in the docs, not "self" which is typically an internal name. Also, "tensor type" is ambiguous and this should refer to dtype.

Maybe an error message like:

"the base given to float_power_ has dtype DTYPE, but the operation's result requires dtype DTYPE"

?

Note also there's not a space between the first base.scalar_type() and "is in the string. A similar printing error can be seen in the error messages below.

is this even correct behavior? Don't we generally check that the computation type is castable to the result type, not that they are the same?

Good point. I extended the Developer FAQ entry to address this explicitly (see here).

So yes, a corrected version of the check would validate that the operation's common dtype is "safe castable" to base's dtype.

In this case, delegating to the out= variant of float_power here would probably save adding special logic for this case. That logic needs to be added to the out= variant (per other comments), anyway.

Ok, since this is a different issue, I filed #50213.

mruberry · 2021-01-06T14:41:49Z

While many of the failures here are in upstream, some are real:

test_float_power_constant_cpu - TestAutogradDeviceTypeCPU
test_autograd.py

Traceback (most recent call last):
  File "/opt/conda/lib/python3.6/site-packages/torch/testing/_internal/common_device_type.py", line 284, in instantiated_test
    raise rte
  File "/opt/conda/lib/python3.6/site-packages/torch/testing/_internal/common_device_type.py", line 279, in instantiated_test
    result = test_fn(self, *args)
  File "test_autograd.py", line 5206, in do_test
    check(name)
  File "test_autograd.py", line 5177, in check
    **kwargs_variable))
RuntimeError: result type ComplexDoublecan't be cast to the desired output type Double

test_float_power_constant_cuda - TestAutogradDeviceTypeCUDA
test_autograd.py

Traceback (most recent call last):
  File "/opt/conda/lib/python3.6/site-packages/torch/testing/_internal/common_utils.py", line 889, in wrapper
    method(*args, **kwargs)
  File "/opt/conda/lib/python3.6/site-packages/torch/testing/_internal/common_utils.py", line 889, in wrapper
    method(*args, **kwargs)
  File "/opt/conda/lib/python3.6/site-packages/torch/testing/_internal/common_device_type.py", line 284, in instantiated_test
    raise rte
  File "/opt/conda/lib/python3.6/site-packages/torch/testing/_internal/common_device_type.py", line 279, in instantiated_test
    result = test_fn(self, *args)
  File "test_autograd.py", line 5206, in do_test
    check(name)
  File "test_autograd.py", line 5177, in check
    **kwargs_variable))
RuntimeError: result type ComplexDoublecan't be cast to the desired output type Double

test_float_power_constant_cpu - test_jit.TestJitGeneratedAutogradCPU
test_jit.py

Traceback (most recent call last):
  File "/opt/conda/lib/python3.6/site-packages/torch/testing/_internal/common_device_type.py", line 284, in instantiated_test
    raise rte
  File "/opt/conda/lib/python3.6/site-packages/torch/testing/_internal/common_device_type.py", line 279, in instantiated_test
    result = test_fn(self, *args)
  File "/var/lib/jenkins/workspace/test/test_jit.py", line 15849, in do_test
    check(inplace_name)
  File "/opt/conda/lib/python3.6/site-packages/torch/testing/_internal/jit_utils.py", line 646, in wrapper
    fn(*args, **kwargs)
  File "/var/lib/jenkins/workspace/test/test_jit.py", line 15842, in check
    check_alias_annotation(name, (self_variable,) + args_variable, kwargs_variable, aten_name=name)
  File "/opt/conda/lib/python3.6/site-packages/torch/testing/_internal/jit_metaprogramming_utils.py", line 477, in check_alias_annotation
    torch._C._jit_check_alias_annotation(CU.the_method.graph, tuple(tensors), aten_name)
RuntimeError: result type ComplexDoublecan't be cast to the desired output type Double

mruberry · 2021-01-06T14:52:22Z

The error message, with its incorrect spacing, comes from:

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

Line 30 in 97c17b4

TORCH_CHECK(at::can_cast(common_dtype, result.scalar_type()),

mruberry · 2021-01-06T14:55:19Z

It's not obvious to me where this failure is coming from.

We could change float_power to use an OpInfo if there's a test issue.

mruberry · 2021-01-06T14:55:52Z

-  auto dtype = (at::isComplexType(base.scalar_type()) || at::isComplexType(exp.scalar_type())) ? at::kComplexDouble : at::kDouble;
-  return at::pow(base.to(dtype), exp.to(dtype));
+  auto dtype = (at::isComplexType(exp.scalar_type()) || base.isComplex()) ? at::kComplexDouble : at::kDouble;
+  base = dtype == at::kComplexDouble ? base.toComplexDouble() : base.toDouble();


Ideally we'd add an output checking helper (or maybe one exists that we can reuse) here (and in the above _out variant). Otherwise if result has the wrong dtype or the wrong device type the error will be throw in pow, and will not correctly identify float_power as the source of the issue. This could be confusing to users.

There should be no functional change here. A couple of reasons here: 1) This function is generally an anti-pattern (#49758) and it is good to minimize its usage in the code base. 2) pow itself has a fair amount of smarts like not broadcasting scalar/tensor combinations and we should defer to it. Differential Revision: [D25786172](https://our.internmc.facebook.com/intern/diff/D25786172)

mruberry

LGTM!

facebook-github-bot · 2021-01-08T17:49:21Z

@gchanan merged this pull request in 88bd69b.

Summary: Pull Request resolved: pytorch#50105 There should be no functional change here. A couple of reasons here: 1) This function is generally an anti-pattern (pytorch#49758) and it is good to minimize its usage in the code base. 2) pow itself has a fair amount of smarts like not broadcasting scalar/tensor combinations and we should defer to it. Test Plan: Imported from OSS Reviewed By: mruberry Differential Revision: D25786172 Pulled By: gchanan fbshipit-source-id: 89de03aa0b900ce011a62911224a5441f15e331a

facebook-github-bot added the cla signed label Jan 5, 2021

gchanan added 2 commits January 5, 2021 14:31

gchanan requested a review from mruberry January 5, 2021 22:36

mruberry reviewed Jan 6, 2021

View reviewed changes

gchanan added 3 commits January 6, 2021 13:18

mruberry self-requested a review January 8, 2021 00:30

mruberry approved these changes Jan 8, 2021

View reviewed changes

facebook-github-bot closed this in 88bd69b Jan 8, 2021

facebook-github-bot added the Merged label Jan 8, 2021

facebook-github-bot deleted the gh/gchanan/348/head branch January 12, 2021 15:14

Conversation

gchanan commented Jan 5, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

facebook-github-bot commented Jan 5, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

💊 CI failures summary and remediations

Uh oh!

mruberry Jan 6, 2021

Choose a reason for hiding this comment

Uh oh!

mruberry Jan 6, 2021

Choose a reason for hiding this comment

Uh oh!

mruberry Jan 6, 2021

Choose a reason for hiding this comment

Uh oh!

gchanan Jan 6, 2021

Choose a reason for hiding this comment

Uh oh!

mruberry Jan 6, 2021

Choose a reason for hiding this comment

Uh oh!

mruberry Jan 6, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gchanan Jan 7, 2021

Choose a reason for hiding this comment

Uh oh!

mruberry commented Jan 6, 2021

Uh oh!

mruberry commented Jan 6, 2021

Uh oh!

mruberry commented Jan 6, 2021

Uh oh!

mruberry Jan 6, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mruberry left a comment

Choose a reason for hiding this comment

Uh oh!

facebook-github-bot commented Jan 8, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

gchanan commented Jan 5, 2021 •

edited

Loading

facebook-github-bot commented Jan 5, 2021 •

edited

Loading

mruberry Jan 6, 2021 •

edited

Loading

mruberry Jan 6, 2021 •

edited

Loading