Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

remove componentwise comparison of complex values in TestCase.assertEqual #63572

Closed
wants to merge 4 commits into from
Closed

remove componentwise comparison of complex values in TestCase.assertEqual #63572

wants to merge 4 commits into from

Conversation

pmeier
Copy link
Collaborator

@pmeier pmeier commented Aug 19, 2021 •
edited by mruberry

Stack from ghstack:

Addresses #61906. Issue will be fixed later in the stack when torch.testing.assert_close got the same treatment.

cc @ezyang @gchanan

Differential Revision: D30633527

@pmeier pmeier mentioned this pull request Aug 19, 2021
Closed
@facebook-github-bot
Copy link
Contributor

facebook-github-bot commented Aug 19, 2021 •
edited

🔗 Helpful links

💊 CI failures summary and remediations

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

  • 3/3 failures introduced in this PR

🕵️ 3 new failures recognized by patterns

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

See CircleCI build pytorch_pure_torch_linux_xenial_py3_6_gcc5_4_build (1/3)

Step: "(Optional) Merge target branch" (full log | diagnosis details | 🔁 rerun)

Automatic merge failed; fix conflicts and then commit the result. 
CONFLICT (add/add): Merge conflict in .circleci/scripts/cpp_doc_push_script.sh
Auto-merging .circleci/scripts/cpp_doc_push_script.sh
CONFLICT (add/add): Merge conflict in .circleci/scripts/binary_macos_build.sh
Auto-merging .circleci/scripts/binary_macos_build.sh
CONFLICT (add/add): Merge conflict in .circleci/config.yml
Auto-merging .circleci/config.yml
CONFLICT (add/add): Merge conflict in .circleci/cimodel/data/pytorch_build_definitions.py
Auto-merging .circleci/cimodel/data/pytorch_build_definitions.py
CONFLICT (add/add): Merge conflict in .bazelrc
Auto-merging .bazelrc
Automatic merge failed; fix conflicts and then commit the result.


Exited with code exit status 1

See CircleCI build pytorch_paralleltbb_linux_xenial_py3_6_gcc5_4_build (2/3)

Step: "(Optional) Merge target branch" (full log | diagnosis details | 🔁 rerun)

Automatic merge failed; fix conflicts and then commit the result. 
CONFLICT (add/add): Merge conflict in .circleci/scripts/cpp_doc_push_script.sh
Auto-merging .circleci/scripts/cpp_doc_push_script.sh
CONFLICT (add/add): Merge conflict in .circleci/scripts/binary_macos_build.sh
Auto-merging .circleci/scripts/binary_macos_build.sh
CONFLICT (add/add): Merge conflict in .circleci/config.yml
Auto-merging .circleci/config.yml
CONFLICT (add/add): Merge conflict in .circleci/cimodel/data/pytorch_build_definitions.py
Auto-merging .circleci/cimodel/data/pytorch_build_definitions.py
CONFLICT (add/add): Merge conflict in .bazelrc
Auto-merging .bazelrc
Automatic merge failed; fix conflicts and then commit the result.


Exited with code exit status 1

See CircleCI build pytorch_parallelnative_linux_xenial_py3_6_gcc5_4_build (3/3)

Step: "(Optional) Merge target branch" (full log | diagnosis details | 🔁 rerun)

Automatic merge failed; fix conflicts and then commit the result. 
CONFLICT (add/add): Merge conflict in .circleci/scripts/cpp_doc_push_script.sh
Auto-merging .circleci/scripts/cpp_doc_push_script.sh
CONFLICT (add/add): Merge conflict in .circleci/scripts/binary_macos_build.sh
Auto-merging .circleci/scripts/binary_macos_build.sh
CONFLICT (add/add): Merge conflict in .circleci/config.yml
Auto-merging .circleci/config.yml
CONFLICT (add/add): Merge conflict in .circleci/cimodel/data/pytorch_build_definitions.py
Auto-merging .circleci/cimodel/data/pytorch_build_definitions.py
CONFLICT (add/add): Merge conflict in .bazelrc
Auto-merging .bazelrc
Automatic merge failed; fix conflicts and then commit the result.


Exited with code exit status 1
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.

Click here to manually regenerate this comment.

pmeier added a commit that referenced this pull request Aug 19, 2021
@pmeier
remove componentwise comparison of complex values in TestCase.assertE…
1da9995 
…qual

ghstack-source-id: cb9f2ff52c2f62fc3fd72290370667f4854b1367
Pull Request resolved: #63572
@pmeier
Update on "remove componentwise comparison of complex values in TestC…
44d0217 
…ase.assertEqual"


Addresses #61906. Issue will be fixed later in the stack when `torch.testing.assert_close` got the same treatment.

[ghstack-poisoned]
pmeier added a commit that referenced this pull request Aug 23, 2021
@pmeier
remove componentwise comparison of complex values in TestCase.assertE…
01de995 
…qual

ghstack-source-id: 476d75b23a46fad7a8baeb0e8395a79824b774f5
Pull Request resolved: #63572
@pmeier
Copy link
Collaborator Author

pmeier commented Aug 23, 2021

There is one issue that materializes for example in test_tensor_creation_ops.TestRandomTensorCreation.test_randn:

>>> t = torch.tensor(0, dtype=torch.complex32)
>>> torch.allclose(t, t)
RuntimeError: "eq_cpu" not implemented for 'ComplexHalf'
>>> t = t.cuda()
>>> torch.allclose(t, t)
RuntimeError: "eq_cuda" not implemented for 'ComplexHalf'

Previously one complex32 was treated as two float16 which prevents this. We could skip complex32 in all tests that compare values, but for the future equality should be supported for these values.

@pmeier
Update on "remove componentwise comparison of complex values in TestC…
db572b7 
…ase.assertEqual"


Addresses #61906. Issue will be fixed later in the stack when `torch.testing.assert_close` got the same treatment.

[ghstack-poisoned]
pmeier added a commit that referenced this pull request Aug 24, 2021
@pmeier
remove componentwise comparison of complex values in TestCase.assertE…
ce891da 
…qual

ghstack-source-id: edde9eb62299b0cf4e8fdeb22ab47c06f8dd89b2
Pull Request resolved: #63572
@pmeier pmeier mentioned this pull request Aug 24, 2021
Closed
@pmeier pmeier marked this pull request as ready for review August 24, 2021 06:00
@pmeier pmeier requested a review from mruberry August 24, 2021 06:00
@pmeier pmeier added module: bc-breaking Related to a BC-breaking change module: testing Issues related to the torch.testing module (not tests) labels Aug 24, 2021
@mruberry
Copy link
Collaborator

Some (one?) of the test failures seem real:

test_randn_cpu_complex32 - TestRandomTensorCreationCPU

Traceback (most recent call last):
  File "/opt/conda/lib/python3.6/site-packages/torch/testing/_internal/common_device_type.py", line 378, in instantiated_test
    raise rte
  File "/opt/conda/lib/python3.6/site-packages/torch/testing/_internal/common_device_type.py", line 373, in instantiated_test
    result = test(self, **param_kwargs)
  File "test_tensor_creation_ops.py", line 3269, in test_randn
    self.assertEqual(res1, res2)
  File "/opt/conda/lib/python3.6/site-packages/torch/testing/_internal/common_utils.py", line 1550, in assertEqual
    exact_device=exact_device)
  File "/opt/conda/lib/python3.6/site-packages/torch/testing/_internal/common_utils.py", line 1415, in _compareTensors
    return _compare_tensors_internal(a, b, rtol=rtol, atol=atol, equal_nan=equal_nan)
  File "/opt/conda/lib/python3.6/site-packages/torch/testing/_core.py", line 118, in _compare_tensors_internal
    if torch.allclose(a, b, rtol=rtol, atol=atol, equal_nan=equal_nan):
RuntimeError: "eq_cpu" not implemented for 'ComplexHalf'

A fix for this could be to implement allclose for complexhalf or to upcast complexhalf tensors in assertEqual/compareTensors/assert_close.

@mruberry
Copy link
Collaborator

cc @zasdfgbnm

@pmeier
Copy link
Collaborator Author

pmeier commented Aug 25, 2021

A fix for this could be to implement allclose for complexhalf or to upcast complexhalf tensors in assertEqual/compareTensors/assert_close.

It is not allclose, but rather equality:

>>> t = torch.tensor(0, dtype=torch.complex32)
>>> t == t
RuntimeError: "eq_cpu" not implemented for 'ComplexHalf'
>>> t = t.cuda()
>>> t == t
RuntimeError: "eq_cuda" not implemented for 'ComplexHalf'

Feels weird that this fundamental method is not there.

@anjali411
Copy link
Contributor

anjali411 commented Aug 25, 2021 •
edited

A fix for this could be to implement allclose for complexhalf or to upcast complexhalf tensors in assertEqual/compareTensors/assert_close.

It is not allclose, but rather equality:

>>> t = torch.tensor(0, dtype=torch.complex32)
>>> t == t
RuntimeError: "eq_cpu" not implemented for 'ComplexHalf'
>>> t = t.cuda()
>>> t == t
RuntimeError: "eq_cuda" not implemented for 'ComplexHalf'

Feels weird that this fundamental method is not there.

There's bare minimum support for ComplexHalf. In fact, we should probably just remove the ComplexHalf dtype until enough functionality is added.

As for the failing tests for this PR, it's safe to disable any test for torch.complex32.

@pmeier
Update on "remove componentwise comparison of complex values in TestC…
2b0074a 
…ase.assertEqual"


Addresses #61906. Issue will be fixed later in the stack when `torch.testing.assert_close` got the same treatment.

cc @ezyang @gchanan

[ghstack-poisoned]
mruberry
mruberry approved these changes Aug 30, 2021
View reviewed changes
Copy link
Collaborator

@mruberry mruberry left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool

@mruberry
Copy link
Collaborator

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

@facebook-github-bot
Copy link
Contributor

@mruberry merged this pull request in 401bbb2.

@facebook-github-bot facebook-github-bot deleted the gh/pmeier/35/head branch September 3, 2021 14:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mruberry mruberry mruberry approved these changes

@anjali411 anjali411 Awaiting requested review from anjali411

Assignees
No one assigned
Labels
cla signed Merged module: bc-breaking Related to a BC-breaking change module: testing Issues related to the torch.testing module (not tests) open source
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@pmeier @facebook-github-bot @mruberry @anjali411 @pytorchbot