Remove unnecessary dtype checks for complex types & disable complex dispatch for CPU min/max pointwise ops by imaginary-person · Pull Request #50465 · pytorch/pytorch

imaginary-person · 2021-01-13T08:42:28Z

PROBLEM DESCRIPTION:

Had not removed dtype checks for complex types in the previous PR (Disable complex dispatch on min/max functions #50347) for this issue.
These type-checks were added in Disables complex min and max #36377, but are no longer necessary,
as we now rely upon dispatch macros to produce error messages.
dtype checks in clamp_max() and clamp_min() for complex inputs had not been removed either.
For min/max pointwise ops in TensorCompareKernel.cpp, complex dispatch had not been removed for min/max functions.

FIX DESCRIPTION:

FIX SUMMARY:

Removed dtype checks added in Disables complex min and max #36377, and added 3 more in TensorCompare.cpp.
Removed dtype checks for complex inputs in clamp_max() and clamp_min().
Disabled complex dispatch for min/max pointwise ops in TensorCompareKernel.cpp.
Error messages in the exceptions raised due to min/max ops not being implemented are now checked for containing the text not support (which can also be present in not supported), or not implemented, so one of them should be a part of error messages, in order for them to be informative.

REASON FOR NOT CHANGING DISPATCH FOR CUDA AND CLAMP OPS:

As for the CUDA min/max operations, their kernels do not seem to be compiled & dispatched for complex types anyway, so no further changes seem to be required. Basically, the dispatch macros currently being used don't have cases for complex types.

For example,

the reduce CUDA ops use AT_DISPATCH_ALL_TYPES_AND2 in ReduceMinMaxKernel.cu, and that macro doesn't allow complex types.
In MinMaxElementwiseKernel.cu, the CUDA pointwise ops use AT_DISPATCH_FLOATING_TYPES_AND2 for non-integral & non-boolean types, and this marco doesn't have a case for complex types either.
clamp CUDA ops use AT_DISPATCH_ALL_TYPES_AND2, which doesn't have a case for complex types.

Similarly, CPU clamp min/max ops use the AT_DISPATCH_ALL_TYPES_AND dispatch macro, which doesn't have a case for complex types.

REASON FOR ADDING 3 dtype CHECKS:
There are a few cases in which the methods corresponding to min_stub() or max_stub() are not called, so dispatch macros don't get invoked, resulting in no exceptions being raised. Hence, dtype checks are necessary at 3 places to raise exceptions:

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

Line 342 in 52dcc72

if (_dimreduce_return_trivial_no_ident(max, self, dim, keepdim, "max")) {
pytorch/aten/src/ATen/native/TensorCompare.cpp

Line 422 in 52dcc72

if (_dimreduce_return_trivial_no_ident(min, self, dim, keepdim, "min")) {
pytorch/aten/src/ATen/native/TensorCompare.cpp

Line 389 in 52dcc72

if (_dimreduce_return_trivial_no_ident(min, self, dim, keepdim, "min") &&

The first dtype check requirement can be verified from the following example Python code based on test_complex_unsupported():

import unittest 
import torch

class MyTestCase(unittest.TestCase): 
  
   def test_1(self):
      t = torch.tensor((1 + 1j), device='cpu', dtype=torch.complex128) 
      with self.assertRaises(Exception): 
         torch.max(t, dim=0)
  
if __name__ == '__main__':  
    unittest.main()

facebook-github-bot · 2021-01-13T09:16:36Z

💊 CI failures summary and remediations

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

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

ci.pytorch.org: 1 failed

Failed: pr/pytorch-linux-bionic-rocm3.10-py3.6

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.

imaginary-person · 2021-01-13T20:16:55Z

All the errors are related to test_complex_unsupported() when the dim parameter is used.
AT_ERROR() also uses TORCH_CHECK() to raise a RuntimeError,
so I'll check why it's not working as intended & will fix the bug after testing test_complex_unsupported() locally as soon as my local debug build would finish.

imaginary-person · 2021-01-14T02:54:16Z

              max_indices.device(), " for indices output");
  dim = maybe_wrap_dim(dim, self.dim());
  if (_dimreduce_return_trivial_no_ident(max, self, dim, keepdim, "max")) {
+    TORCH_CHECK(!self.is_complex(), "max does not support complex inputs.");


It's worth noting that dispatch handlers are invoked in the methods registered for min_stub & max_stub.
However, the tests fail in some conditional cases such as this one, in which these stubs are not being called!

We can't rely upon dispatch macros to raise exceptions if they're never invoked in the first place!
So, in these corner cases, we need dtype checks.

Great, thanks for investigating!

codecov · 2021-01-14T05:52:50Z

Codecov Report

Merging #50465 (d849f83) into master (ca5d961) will decrease coverage by 0.00%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master   #50465      +/-   ##
==========================================
- Coverage   80.73%   80.73%   -0.01%     
==========================================
  Files        1910     1910              
  Lines      207182   207158      -24     
==========================================
- Hits       167270   167244      -26     
- Misses      39912    39914       +2

imaginary-person · 2021-01-14T19:45:14Z

💊 CI failures summary and remediations

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

1/1 failures possibly* introduced in this PR

1/1 non-CircleCI failure(s)

ci.pytorch.org: 1 failed

Failed: pr/pytorch-linux-bionic-rocm3.10-py3.6

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

This failure is not due to the code in this PR, as Jenkins builds for other PRs are still failing due to the same cause.
For example, https://ci.pytorch.org/jenkins/job/pytorch-builds/job/pytorch-linux-bionic-rocm3.10-py3.6-trigger/3500/

The error messages returned for complex inputs to min/max ops should be informative. Added checks for clamp as well.

ngimel · 2021-01-15T00:58:54Z

This is looking really good! Can you please also remove dtype checks for clamp in UnaryOps.cpp, and rely on dispatch throwing the errors? Dispatch is not enabled for compelx currently, so as far as I can see no dispatch changes are necessary.

imaginary-person · 2021-01-15T01:01:27Z

This is looking really good! Can you please also remove dtype checks for clamp in UnaryOps.cpp, and rely on dispatch throwing the errors? Dispatch is not enabled for compelx currently, so as far as I can see no dispatch changes are necessary.

Thanks for your advice! I'll do that right away.

Removed dtype checks for complex types as the dispatch macro used (AT_DISPATCH_ALL_TYPES_AND) doesn't allow complex types, and would default to error anyway. So, the dispatch macro can be relied upon for raising an exception for unsupported complex types.

ngimel

Let's wait for CI, otherwise looks good.

imaginary-person · 2021-01-15T19:36:04Z

Hello @ngimel, it seems that the Codecov/patch check is insignificant because it's related to a line of code that does a dtype check for complex types if a particular conditional statement is true, as no dispatch macro is invoked for that condition.

The Codecov/patch check failed because no test cases check for that condition, so that line of code was never visited at runtime.

imaginary-person · 2021-01-15T19:39:29Z

💊 CI failures summary and remediations

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

1/1 failures possibly* introduced in this PR

1/1 non-CircleCI failure(s)

ci.pytorch.org: 1 failed

Failed: pr/pytorch-linux-bionic-rocm3.10-py3.6

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

This failure is not due to the code in this PR, as Jenkins builds for other PRs are still failing due to the same cause.
For example, https://ci.pytorch.org/jenkins/job/pytorch-builds/job/pytorch-linux-bionic-rocm3.10-py3.6-trigger/3500/

This unrelated issue was fixed in #50518

facebook-github-bot

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

imaginary-person · 2021-01-17T02:30:11Z

Let's wait for CI, otherwise looks good.

Hello @ngimel, please let me know if I should add a test pertaining to torch._aminmax() for complex inputs in order to pass the Codecov/patch check. Thanks!

Tests to ensure that users receive a runtime exception for_aminmax() currently not being supported for complex types. A dispatch macro handles all cases except the case in which the tensor has a single element, and the dimension has been provided as an argument. In that particular case, a dtype check generates an exception. That dtype check was also added in TensorCompare.cpp in this PR.

Delete trailing spaces in comment

Remove whitespace from empty line

Remove extra whitespace

facebook-github-bot

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

ngimel · 2021-01-17T22:01:53Z

Thanks for adding the test!

imaginary-person · 2021-01-18T02:23:46Z

Thanks for adding the test!

Thanks for the opportunity to contribute, @ngimel!
Codecov/patch is passing now, but some other internal FB tests still seem to be failing.
I'm unable to see the failures, but if required, I'll make any necessary changes. Thanks!

ngimel · 2021-01-18T04:56:51Z

Internal failures are unrelated, I'm trying to land now. Thanks for the contribution!

facebook-github-bot · 2021-01-18T06:04:05Z

@ngimel merged this pull request in 3f052ba.

…ispatch for CPU min/max pointwise ops (pytorch#50465) Summary: Fixes pytorch#50064 **PROBLEM DESCRIPTION:** 1. Had not removed dtype checks for complex types in the previous PR (pytorch#50347) for this issue. These type-checks were added in pytorch#36377, but are no longer necessary, as we now rely upon dispatch macros to produce error messages. 2. dtype checks in `clamp_max()` and `clamp_min()` for complex inputs had not been removed either. 3. For min/max pointwise ops in TensorCompareKernel.cpp, complex dispatch had not been removed for min/max functions. ### **FIX DESCRIPTION:** **FIX SUMMARY:** 1. Removed dtype checks added in pytorch#36377, and added 3 more in TensorCompare.cpp. 2. Removed dtype checks for complex inputs in `clamp_max()` and `clamp_min()`. 3. Disabled complex dispatch for min/max pointwise ops in TensorCompareKernel.cpp. 4. Error messages in the exceptions raised due to min/max ops not being implemented are now checked for containing the text _not support_ (which can also be present in _not supported_), or _not implemented_, so one of them should be a part of error messages, in order for them to be informative. **REASON FOR NOT CHANGING DISPATCH FOR CUDA AND CLAMP OPS**: As for the CUDA min/max operations, their kernels do not seem to be compiled & dispatched for complex types anyway, so no further changes seem to be required. Basically, the dispatch macros currently being used don't have cases for complex types. For example, 1. the reduce CUDA ops use [AT_DISPATCH_ALL_TYPES_AND2 (https://github.com/pytorch/pytorch/commit/c6d70b035970375e5ebd2682195748013af91b4e)](https://github.com/pytorch/pytorch/blob/master/aten/src/ATen/Dispatch.h#L548-L575) in [ReduceMinMaxKernel.cu](https://github.com/pytorch/pytorch/blob/master/aten/src/ATen/native/cuda/ReduceMinMaxKernel.cu), and that macro doesn't allow complex types. 2. In [MinMaxElementwiseKernel.cu](https://github.com/pytorch/pytorch/blob/master/aten/src/ATen/native/cuda/MaxMinElementwiseKernel.cu), the CUDA pointwise ops use [`AT_DISPATCH_FLOATING_TYPES_AND2 (https://github.com/pytorch/pytorch/commit/c6d70b035970375e5ebd2682195748013af91b4e)`](https://github.com/pytorch/pytorch/blob/master/aten/src/ATen/Dispatch.h#L240-L263) for non-integral & non-boolean types, and this marco doesn't have a case for complex types either. 3. [clamp CUDA ops](https://github.com/pytorch/pytorch/blob/master/aten/src/ATen/native/cuda/UnaryOpsKernel.cu#L170-L211) use `AT_DISPATCH_ALL_TYPES_AND2 (https://github.com/pytorch/pytorch/commit/c6d70b035970375e5ebd2682195748013af91b4e)`, which doesn't have a case for complex types. Similarly, [CPU clamp min/max ops](https://github.com/pytorch/pytorch/blob/master/aten/src/ATen/native/cpu/UnaryOpsKernel.cpp#L428-L458) use the `AT_DISPATCH_ALL_TYPES_AND `dispatch macro, which doesn't have a case for complex types. **REASON FOR ADDING 3 dtype CHECKS:** There are a few cases in which the methods corresponding to `min_stub()` or `max_stub()` are not called, so dispatch macros don't get invoked, resulting in no exceptions being raised. Hence, `dtype` checks are necessary at 3 places to raise exceptions: 1. https://github.com/pytorch/pytorch/blob/52dcc7299925de055d330781d2fe0dad71182829/aten/src/ATen/native/TensorCompare.cpp#L342 2. https://github.com/pytorch/pytorch/blob/52dcc7299925de055d330781d2fe0dad71182829/aten/src/ATen/native/TensorCompare.cpp#L422 3. https://github.com/pytorch/pytorch/blob/52dcc7299925de055d330781d2fe0dad71182829/aten/src/ATen/native/TensorCompare.cpp#L389 The first dtype check requirement can be verified from the following example Python code based on `test_complex_unsupported()`: ``` import unittest import torch class MyTestCase(unittest.TestCase): def test_1(self): t = torch.tensor((1 + 1j), device='cpu', dtype=torch.complex128) with self.assertRaises(Exception): torch.max(t, dim=0) if __name__ == '__main__': unittest.main() ``` Pull Request resolved: pytorch#50465 Reviewed By: mruberry Differential Revision: D25938106 Pulled By: ngimel fbshipit-source-id: 95e2df02ba8583fa3ce87d4a2fdcd60b912dda46

Remove dtype checks for complex

7ebb4db

imaginary-person mentioned this pull request Jan 13, 2021

Complex dispatch should be disabled on min/max functions #50064

Closed

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

pytorchbot added the open source label Jan 13, 2021

Revise test_maximum_minimum_complex()

9eb1fb2

anjali411 requested a review from ngimel January 13, 2021 15:01

imaginary-person marked this pull request as draft January 13, 2021 17:16

Remove complex dispatch from CPU min/max pointwise ops

cef471a

imaginary-person changed the title ~~Remove dtype checks for complex types~~ Remove dtype checks for complex types & disable complex dispatch for CPU min/max pointwise ops Jan 13, 2021

Add dtype checks for min/max where stubs are not called

52dcc72

imaginary-person commented Jan 14, 2021

View reviewed changes

imaginary-person marked this pull request as ready for review January 14, 2021 06:00

imaginary-person changed the title ~~Remove dtype checks for complex types & disable complex dispatch for CPU min/max pointwise ops~~ Remove unnecessary dtype checks for complex types & disable complex dispatch for CPU min/max pointwise ops Jan 14, 2021

Check error messages returned for complex min/max

277d8e7

The error messages returned for complex inputs to min/max ops should be informative. Added checks for clamp as well.

ngimel approved these changes Jan 15, 2021

View reviewed changes

facebook-github-bot reviewed Jan 16, 2021

View reviewed changes

imaginary-person added 4 commits January 17, 2021 00:20

Delete trailing spaces in comment

6d4200d

Delete trailing spaces in comment

Remove whitespace from empty line

bfed8e0

Remove whitespace from empty line

Remove extra whitespace

d849f83

Remove extra whitespace

facebook-github-bot reviewed Jan 17, 2021

View reviewed changes

facebook-github-bot closed this in 3f052ba Jan 18, 2021

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

Conversation

imaginary-person commented Jan 13, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

FIX DESCRIPTION:

Uh oh!

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

Uh oh!

💊 CI failures summary and remediations

ci.pytorch.org: 1 failed

Uh oh!

imaginary-person commented Jan 13, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

imaginary-person Jan 14, 2021

Choose a reason for hiding this comment

Uh oh!

ngimel Jan 14, 2021

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Codecov Report

Uh oh!

imaginary-person commented Jan 14, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

💊 CI failures summary and remediations

ci.pytorch.org: 1 failed

Uh oh!

ngimel commented Jan 15, 2021

Uh oh!

imaginary-person commented Jan 15, 2021

Uh oh!

ngimel left a comment

Choose a reason for hiding this comment

Uh oh!

imaginary-person commented Jan 15, 2021

Uh oh!

imaginary-person commented Jan 15, 2021

💊 CI failures summary and remediations

ci.pytorch.org: 1 failed

Uh oh!

facebook-github-bot left a comment

Choose a reason for hiding this comment

Uh oh!

imaginary-person commented Jan 17, 2021

Uh oh!

facebook-github-bot left a comment

Choose a reason for hiding this comment

Uh oh!

ngimel commented Jan 17, 2021

Uh oh!

imaginary-person commented Jan 18, 2021

Uh oh!

ngimel commented Jan 18, 2021

Uh oh!

facebook-github-bot commented Jan 18, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

imaginary-person commented Jan 13, 2021 •

edited

Loading

facebook-github-bot commented Jan 13, 2021 •

edited

Loading

imaginary-person commented Jan 13, 2021 •

edited

Loading

codecov Bot commented Jan 14, 2021 •

edited

Loading

imaginary-person commented Jan 14, 2021 •

edited

Loading