Migrate le/gt/ge/eq/ne from the TH to Aten. Added support of type promotion. by ifedan · Pull Request #26981 · pytorch/pytorch

ifedan · 2019-09-27T18:04:58Z

#24606 Migrate ne and ne_ from the TH to Aten (CUDA)
#24740 Migrate ne and ne_ from the TH to Aten (CPU)
#24573 Migrate gt and gt_ from the TH to Aten (CUDA)
#24709 Migrate gt and gt_ from the TH to Aten (CPU)
#24556 Migrate eq and eq_ from the TH to Aten (CUDA)
#24696 Migrate eq and eq_ from the TH to Aten (CPU)
#24568 Migrate ge and ge_ from the TH to Aten (CUDA)
#24703 Migrate ge and ge_ from the TH to Aten (CPU)
#24582 Migrate le and le_ from the TH to Aten (CUDA)
#24719 Migrate le and le_ from the TH to Aten (CPU)

Performance characteristics are similar to #25998

This PR migrates comparison ops from TH to ATen and adds type promotion in the same way as in #25998

zou3519 · 2019-09-27T18:37:54Z

You should note in the commit message that this adds some type promotion behavior

zou3519 · 2019-09-27T18:57:48Z

"All the comparison operation will follow type promotion described" this makes it vague about if we've added type promotion or not. It would be good to be explicit and say "This PR migrates comparison ops from TH to ATen and adds type promotion in the same way as in #25998". Would also be good to update the PR title to say something about type promotion.

zou3519 · 2019-09-27T19:00:09Z

aten/src/THC/THCTensorMathCompare.cuh

-};
-
 template<typename ScalarTypeOut, typename ScalarType, typename TensorTypeOut, typename TensorType, class Op>
 void THC_logicalValue(THCState *state,


I'm not sure if we still need this. However, let's worry about that in the future and not in this PR.

zou3519 · 2019-09-27T19:03:23Z

test/test_torch.py

    def test_inplace_comparison_ops_require_inputs_have_same_dtype(self):
        with self.assertRaisesRegex(RuntimeError, 'Expected object of scalar type'):
            torch.tensor([1], dtype=torch.int).lt_(torch.tensor([2], dtype=torch.long))
+            torch.tensor([1], dtype=torch.int).le_(torch.tensor([2], dtype=torch.long))


Please clean up these tests by not repeating yourself. If we make this a for loop, it only needs to be 3 lines instead of 6

zou3519

We can repeat ourselves less in the tests but otherwise lgtm. Good work in deleting all those lines of code!

facebook-github-bot

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

ifedan · 2019-09-27T21:01:29Z

We can repeat ourselves less in the tests but otherwise lgtm. Good work in deleting all those lines of code!

Test changed based on comment

facebook-github-bot

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

…motion. (#26981) Summary: pytorch/pytorch#24606 Migrate ne and ne_ from the TH to Aten (CUDA) pytorch/pytorch#24740 Migrate ne and ne_ from the TH to Aten (CPU) pytorch/pytorch#24573 Migrate gt and gt_ from the TH to Aten (CUDA) pytorch/pytorch#24709 Migrate gt and gt_ from the TH to Aten (CPU) pytorch/pytorch#24556 Migrate eq and eq_ from the TH to Aten (CUDA) pytorch/pytorch#24696 Migrate eq and eq_ from the TH to Aten (CPU) pytorch/pytorch#24568 Migrate ge and ge_ from the TH to Aten (CUDA) pytorch/pytorch#24703 Migrate ge and ge_ from the TH to Aten (CPU) pytorch/pytorch#24582 Migrate le and le_ from the TH to Aten (CUDA) pytorch/pytorch#24719 Migrate le and le_ from the TH to Aten (CPU) Performance characteristics are similar to pytorch/pytorch#25998 This PR migrates comparison ops from TH to ATen and adds type promotion in the same way as in pytorch/pytorch#25998 Pull Request resolved: pytorch/pytorch#26981 Differential Revision: D17635651 Pulled By: ifedan fbshipit-source-id: 6ec7615207f5c248a6dd85fc54c25bd5e6d328e6

facebook-github-bot · 2019-09-28T01:08:13Z

@ifedan merged this pull request in 541de7e.

mruberry · 2019-09-28T04:10:52Z

~~This PR looks super cool but I think it was breaking cuDNN test builds. I had to unland it. I think you just need to modify test_overwrite_module_params_on_conversion_cpu_cuda.~~

Edit/Update: The revert fixed the cuDNN builds. Looks like this PR did break the cuDNN test builds.

…motion. (#27017) Summary: #26981 Pull Request resolved: #27017 Differential Revision: D17651454 Pulled By: ifedan fbshipit-source-id: c6313caa11598a0ef160e1c6d2f3c33d03ce80c5

…motion. (#27017) Summary: pytorch/pytorch#26981 Pull Request resolved: pytorch/pytorch#27017 Differential Revision: D17651454 Pulled By: ifedan fbshipit-source-id: c6313caa11598a0ef160e1c6d2f3c33d03ce80c5

…motion. (pytorch#26981) Summary: pytorch#24606 Migrate ne and ne_ from the TH to Aten (CUDA) pytorch#24740 Migrate ne and ne_ from the TH to Aten (CPU) pytorch#24573 Migrate gt and gt_ from the TH to Aten (CUDA) pytorch#24709 Migrate gt and gt_ from the TH to Aten (CPU) pytorch#24556 Migrate eq and eq_ from the TH to Aten (CUDA) pytorch#24696 Migrate eq and eq_ from the TH to Aten (CPU) pytorch#24568 Migrate ge and ge_ from the TH to Aten (CUDA) pytorch#24703 Migrate ge and ge_ from the TH to Aten (CPU) pytorch#24582 Migrate le and le_ from the TH to Aten (CUDA) pytorch#24719 Migrate le and le_ from the TH to Aten (CPU) Performance characteristics are similar to pytorch#25998 This PR migrates comparison ops from TH to ATen and adds type promotion in the same way as in pytorch#25998 Pull Request resolved: pytorch#26981 Differential Revision: D17635651 Pulled By: ifedan fbshipit-source-id: 6ec7615207f5c248a6dd85fc54c25bd5e6d328e6

…motion. (pytorch#27017) Summary: pytorch#26981 Pull Request resolved: pytorch#27017 Differential Revision: D17651454 Pulled By: ifedan fbshipit-source-id: c6313caa11598a0ef160e1c6d2f3c33d03ce80c5

Migrate le/gt/ge/eq/ne from the TH to Aten

b4855e8

Migrate le/gt/ge/eq/ne from the TH to Aten

acd9f21

ifedan marked this pull request as ready for review September 27, 2019 18:25

Migrate le/gt/ge/eq/ne from the TH to Aten

dc5fc8a

ifedan added this to the 1.3 milestone Sep 27, 2019

Migrate le/gt/ge/eq/ne from the TH to Aten

3a0bfbb

ifedan changed the title ~~Migrate le/gt/ge/eq/ne from the TH to Aten~~ Migrate le/gt/ge/eq/ne from the TH to Aten. Added support of type promotion. Sep 27, 2019

zou3519 reviewed Sep 27, 2019

View reviewed changes

zou3519 approved these changes Sep 27, 2019

View reviewed changes

ifedan added 2 commits September 27, 2019 15:27

Migrate le/gt/ge/eq/ne from the TH to Aten

bee263f

Migrate le/gt/ge/eq/ne from the TH to Aten

05d09f3

facebook-github-bot reviewed Sep 27, 2019

View reviewed changes

Changes based on PR review

f86efcc

facebook-github-bot reviewed Sep 27, 2019

View reviewed changes

facebook-github-bot closed this in 541de7e Sep 28, 2019

facebook-github-bot added the merged label Sep 28, 2019

mruberry reopened this Sep 28, 2019

ifedan added 2 commits September 28, 2019 08:37

Merge branch 'master' into comparison_ops

bd75c4b

Fix test

cc195a1

ifedan closed this Sep 28, 2019

ifedan mentioned this pull request Sep 28, 2019

Migrate le/gt/ge/eq/ne from the TH to Aten. Added support of type promotion. #27017

Closed

ifedan mentioned this pull request Sep 29, 2019

Ambiguous torch.Tensor int vs float comparison #22301

Closed

mruberry added the Merged label Oct 28, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migrate le/gt/ge/eq/ne from the TH to Aten. Added support of type promotion.#26981

Migrate le/gt/ge/eq/ne from the TH to Aten. Added support of type promotion.#26981
ifedan wants to merge 9 commits intopytorch:masterfrom
ifedan:comparison_ops

ifedan commented Sep 27, 2019 •

edited

Loading

Uh oh!

zou3519 commented Sep 27, 2019

Uh oh!

zou3519 commented Sep 27, 2019

Uh oh!

zou3519 Sep 27, 2019

Uh oh!

zou3519 Sep 27, 2019

Uh oh!

zou3519 left a comment

Uh oh!

facebook-github-bot left a comment

Uh oh!

ifedan commented Sep 27, 2019

Uh oh!

facebook-github-bot left a comment

Uh oh!

facebook-github-bot commented Sep 28, 2019

Uh oh!

mruberry commented Sep 28, 2019 •

edited

Loading

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Conversation

ifedan commented Sep 27, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zou3519 commented Sep 27, 2019

Uh oh!

zou3519 commented Sep 27, 2019

Uh oh!

zou3519 Sep 27, 2019

Choose a reason for hiding this comment

Uh oh!

zou3519 Sep 27, 2019

Choose a reason for hiding this comment

Uh oh!

zou3519 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!

ifedan commented Sep 27, 2019

Uh oh!

facebook-github-bot left a comment

Choose a reason for hiding this comment

Uh oh!

facebook-github-bot commented Sep 28, 2019

Uh oh!

mruberry commented Sep 28, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

ifedan commented Sep 27, 2019 •

edited

Loading

mruberry commented Sep 28, 2019 •

edited

Loading