Returns float tensors for complex inputs to abs by mruberry · Pull Request #35871 · pytorch/pytorch

mruberry · 2020-04-02T08:11:50Z

Per title. A test is added to test_type_promotion for the behavior. This behavior is consistent with NumPy's.

For complex inputs to abs the result is cast to float after the computation since the computation of abs must be performed on the original complex tensor. While std::abs returns a float value when called on complex inputs, returning a FloatTensor directly would require additional loop instantiations in TensorIterator. This may be worthwhile to pursue in the future.

Resolves #33567

dr-ci · 2020-04-02T08:13:08Z

💊 Build failures summary and remediations

As of commit f188dd5 (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.

See how this bot performed.

This comment has been revised 41 times.

mruberry · 2020-04-02T19:24:06Z

To satisfy the clang-format check I had to format shape_analysis.cpp. The relevant change is:

// Requirements:
//   dims           : preserved
//   scalar type    : preserved, except complex maps to float
//   device         : preserved
//   tensor inputs  : 1
//   tensor outputs : 1
// Additionally:
//   - First input should be the only tensor input
static const register_formula_for simple_unary_ops_complex_to_float{
   {
        "aten::abs(Tensor self) -> Tensor",
   },
   [](Node* node) -> type_vec_t {
      if (auto type = node->input(0)->type()->cast<TensorType>()) {
       const auto scalar_type = *(type->scalarType());
       if (isComplexType(scalar_type)) {
         const auto out_type = c10::toValueType(scalar_type);
         return type_vec_t{
             type->withScalarType(out_type)->dimensionedOnly()};
       }
     }
     return type_vec_t{};
   }};

ezyang · 2020-04-03T14:33:48Z

The clang format is a strict downgrade. Let me talk to the people who added the clang format.

anjali411 · 2020-04-04T21:16:15Z

looks good. Ideally we wouldn't want to be able to avoid the extra copy at the end but currently it seems cpu_kernel_vec also does a copy (iter.cast_outputs()) if the out dtype is not same as input dtype.
@mruberry I think we should also update

pytorch/test/test_type_promotion.py

Line 213 in 8a6173e

if dtype == torch.complex64:

and

pytorch/torch/testing/_internal/common_utils.py

Lines 850 to 858 in 7ee88d6

    
           # TODO: modify abs to return float/double for ComplexFloat/ComplexDouble 
        
           if diff.is_signed() and diff.dtype != torch.int8: 
        
               diff = diff.abs() 
        
               # if diff is complex, the imaginary component for diff will be 0 
        
               # from the previous step, hence converting it to float and double is fine. 
        
               if diff.dtype == torch.complex64: 
        
                   diff = diff.to(torch.float) 
        
               elif diff.dtype == torch.complex128: 
        
                   diff = diff.to(torch.double)

:D

…bles_complex_abs

…orch into disables_complex_abs

anjali411

Thanks for working on this :)

…bles_complex_abs

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-04-10T16:12:43Z

@mruberry merged this pull request in 3aeb2b1.

mruberry · 2020-04-11T19:32:19Z

This was reverted since it appears to break a production model.

…bles_complex_abs

mruberry · 2020-04-11T22:09:33Z

Break in production model was due to an error in shape propagation. The shape propagation code is poorly tested by the OSS CI, unfortunately, and detecting errors in it via model breaks is our best defense. Following up with @houseroad about how to run the diff on more models to ensure the shape propagation logic is now correct.

Since we now know that shape propagation issues can pass CI and fail on actual models, the pattern @houseroad recommends should probably be mandatory for all shape prop changes.

facebook-github-bot

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

Summary: Per title. A test is added to test_type_promotion for the behavior. This behavior is consistent with NumPy's. For complex inputs to `abs` the result is cast to float after the computation since the computation of abs must be performed on the original complex tensor. While `std::abs` returns a float value when called on complex inputs, returning a FloatTensor directly would require additional loop instantiations in TensorIterator. This may be worthwhile to pursue in the future. Pull Request resolved: pytorch#35871 Differential Revision: D20961711 Pulled By: mruberry fbshipit-source-id: 232f62cf64caa4154eb2194969efa51d2082d842

facebook-github-bot

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

mruberry · 2020-04-16T02:32:50Z

Tested using @houseroad's run_all_model_execution_test.sh test. Thanks @houseroad!

Summary: Per title. A test is added to test_type_promotion for the behavior. This behavior is consistent with NumPy's. For complex inputs to `abs` the result is cast to float after the computation since the computation of abs must be performed on the original complex tensor. While `std::abs` returns a float value when called on complex inputs, returning a FloatTensor directly would require additional loop instantiations in TensorIterator. This may be worthwhile to pursue in the future. Pull Request resolved: pytorch#35871 Differential Revision: D20961711 Pulled By: mruberry fbshipit-source-id: 232f62cf64caa4154eb2194969efa51d2082d842

Summary: Per title. A test is added to test_type_promotion for the behavior. This behavior is consistent with NumPy's. For complex inputs to `abs` the result is cast to float after the computation since the computation of abs must be performed on the original complex tensor. While `std::abs` returns a float value when called on complex inputs, returning a FloatTensor directly would require additional loop instantiations in TensorIterator. This may be worthwhile to pursue in the future. Pull Request resolved: pytorch#35871 Differential Revision: D20984456 Pulled By: mruberry fbshipit-source-id: 226445178f92f2b0292e92578656d98674a6aa20

Returns float tensors for complex inputs to abs

a1e4d7c

mruberry added module: complex Related to complex number support in PyTorch module: numpy Related to numpy support, and also numpy compatibility of our operators labels Apr 2, 2020

Adds shape prop, updates test to use from_numpy(complex)

ac3af98

mruberry requested a review from apaszke as a code owner April 2, 2020 19:07

Mike Ruberry added 3 commits April 2, 2020 12:11

Adds exact_dtype kwarg

985a4aa

clang format

318558d

clang format

3d2d10a

mruberry requested review from anjali411 and gchanan and removed request for apaszke April 2, 2020 19:24

anjali411 requested a review from ezyang April 2, 2020 20:01

ngimel mentioned this pull request Apr 4, 2020

Implement Complex Support that allows Complex Tests to run in test_torch.py #36029

Closed

5 tasks

anjali411 reviewed Apr 4, 2020

View reviewed changes

Comment thread torch/testing/_internal/common_utils.py Outdated

Mike Ruberry and others added 6 commits April 6, 2020 19:24

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

4a39e22

…bles_complex_abs

Removes unnecessary complex abs workarounds, updates clang-format

1575b5d

Removes unnecessary comment

e6f89e3

clang format strikes back

4046223

Merge branch 'disables_complex_abs' of https://github.com/pytorch/pyt…

3209e54

…orch into disables_complex_abs

fix

7dcaeae

anjali411 approved these changes Apr 7, 2020

View reviewed changes

mruberry commented Apr 7, 2020

View reviewed changes

Comment thread aten/src/ATen/native/UnaryOps.cpp

mruberry commented Apr 8, 2020

View reviewed changes

Comment thread aten/src/ATen/native/UnaryOps.cpp Outdated

mruberry commented Apr 8, 2020

View reviewed changes

Comment thread test/test_type_promotion.py

Mike Ruberry added 2 commits April 9, 2020 01:14

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

f9e9e2e

…bles_complex_abs

Updates per review

f69ae7d

gchanan reviewed Apr 9, 2020

View reviewed changes

Comment thread aten/src/ATen/native/UnaryOps.cpp Outdated

gchanan approved these changes Apr 9, 2020

View reviewed changes

Mike Ruberry added 2 commits April 9, 2020 21:20

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

f87901c

…bles_complex_abs

Updates per review

f781afc

facebook-github-bot reviewed Apr 10, 2020

View reviewed changes

facebook-github-bot closed this in 3aeb2b1 Apr 10, 2020

facebook-github-bot added the merged label Apr 10, 2020

mruberry reopened this Apr 11, 2020

Mike Ruberry added 3 commits April 11, 2020 12:56

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

326384c

…bles_complex_abs

shape analysis fix

742c7f5

clang format

f705a08

mruberry removed the merged label Apr 12, 2020

facebook-github-bot reviewed Apr 12, 2020

View reviewed changes

Merges with master

f188dd5

facebook-github-bot reviewed Apr 15, 2020

View reviewed changes

facebook-github-bot closed this in d0c925f Apr 16, 2020

mruberry deleted the disables_complex_abs branch June 18, 2020 05:18

mruberry added the Merged label Oct 28, 2020

Conversation

mruberry commented Apr 2, 2020 • edited by anjali411 Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

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

Uh oh!

💊 Build failures summary and remediations

Uh oh!

mruberry commented Apr 2, 2020

Uh oh!

ezyang commented Apr 3, 2020

Uh oh!

anjali411 commented Apr 4, 2020

Uh oh!

Uh oh!

anjali411 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

facebook-github-bot left a comment

Choose a reason for hiding this comment

Uh oh!

facebook-github-bot commented Apr 10, 2020

Uh oh!

mruberry commented Apr 11, 2020

Uh oh!

mruberry commented Apr 11, 2020

Uh oh!

facebook-github-bot 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!

mruberry commented Apr 16, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

mruberry commented Apr 2, 2020 •

edited by anjali411

Loading

dr-ci Bot commented Apr 2, 2020 •

edited

Loading