Update foreach binary ops with a scalar list by izdeby · Pull Request #49249 · pytorch/pytorch

izdeby · 2020-12-11T19:28:15Z

Stack from ghstack:

wip #52450 wip
[wip] Replace optimizers in torch.optim with the ones from torch.optim._multi_tensor #49039 [wip] Replace optimizers in torch.optim with the ones from torch.optim._multi_tensor
[WIP] Add type promotion to a fast path for foreach APIs #52449 Add type promotion to a fast path for foreach APIs
[WIP] Support broadcasting via slow path #52448 Support broadcasting via slow path
[wip] Pushed unsupported scenarios to slow path #48224 [wip] Pushed unsupported scenarios to slow path
Updated foreach pointwise ops to support complex and bloat16 on CUDA #51170 Updated foreach pointwise ops to support complex and bloat16 on CUDA
Update foreach max/min #49714 Update foreach max/min
Refactor foreach pointwise ops tests to use OpInfo #51060 Refactor foreach pointwise ops tests to use OpInfo
Refactor foreach binary ops tests with tensor lists to use OpInfo #51059 Refactor foreach binary ops tests with tensor lists to use OpInfo
Refactor foreach binary ops tests with scalars to use OpInfo #51058 Refactor foreach binary ops tests with scalars to use OpInfo
Refactor foreach unary ops tests to use OpInfo #49712 Refactor foreach unary ops tests to use OpInfo
Update foreach binary ops with a single scalar and list of tensors #49250 Update foreach binary ops with a single scalar and list of tensors
Update foreach binary ops with a scalar list #49249 Update foreach binary ops with a scalar list
Refactor ForeachUtils.h #51131 Refactor ForeachUtils.h

Differential Revision: D25502939

Update logic for a set of foreach APIs with a scalar list

Update binary ops to support boolean type
Update sub ops to throw an error in case of bool subtraction
Update division op to check for type promotion.
Update tests

[ghstack-poisoned]

Differential Revision: [D25502939](https://our.internmc.facebook.com/intern/diff/D25502939) -------- - Update binary ops to support boolean type - Update sub ops to throw an error in case of bool subtraction - Update division op to check for type promotion. - Update tests - Update ForeachUtils.h with division check. [ghstack-poisoned]

zou3519

Questions about behavior

aten/src/ATen/native/ForeachUtils.h

test/test_foreach.py

Differential Revision: [D25502939](https://our.internmc.facebook.com/intern/diff/D25502939) -------- - Update binary ops to support boolean type - Update sub ops to throw an error in case of bool subtraction - Update division op to check for type promotion. - Update tests - Update ForeachUtils.h with division check. [ghstack-poisoned]

…ils.h" Differential Revision: [D25502939](https://our.internmc.facebook.com/intern/diff/D25502939) -------- - Update binary ops to support boolean type - Update sub ops to throw an error in case of bool subtraction - Update division op to check for type promotion. - Update tests - Update ForeachUtils.h with division check and refactored `can_use_fast_route` methods. [ghstack-poisoned]

Differential Revision: [D25502939](https://our.internmc.facebook.com/intern/diff/D25502939) -------- - Update binary ops to support boolean type - Update sub ops to throw an error in case of bool subtraction - Update division op to check for type promotion. - Update tests [ghstack-poisoned]

izdeby · 2021-02-18T20:04:23Z

I haven't read through #49250 (the next PR in this stack) but I wanted to ask first: What is the difference between #49250 and this PR? Both of the descriptions look the same:

This PR
Update binary ops to support boolean type
Update sub ops to throw an error in case of bool subtraction
Update division op to check for type promotion.
Update tests
#49250
Update foreach_sub error checking
Update division logic to check for type promotion
Updated tests

In this PR, we are updating the sub/div logic for APIs that work with scalar lists and in the next one APIs with a scalar and/or tensor list. Updating names and description for both PRs

Differential Revision: [D25502939](https://our.internmc.facebook.com/intern/diff/D25502939) -------- Update logic for 2 set of foreach APIs with a scalar list - Update binary ops to support boolean type - Update sub ops to throw an error in case of bool subtraction - Update division op to check for type promotion. - Update tests [ghstack-poisoned]

Differential Revision: [D25502939](https://our.internmc.facebook.com/intern/diff/D25502939) -------- Update logic for a set of foreach APIs with a scalar list - Update binary ops to support boolean type - Update sub ops to throw an error in case of bool subtraction - Update division op to check for type promotion. - Update tests [ghstack-poisoned]

zou3519

This looks pretty reasonable, it's great we are deleting all the special cases in the testing logic. I had some comments around the tests

zou3519 · 2021-02-25T22:37:47Z

aten/src/ATen/native/cuda/ForeachBinaryOpScalarList.cu

+// In the case of division, integer inputs will result in float. 
+// Currently multi tensor apply can only return result of the same type as input.


nit: It would be good to mention "This does not use FOREACH_BINARY_OP_SCALARLIST because ". It's easy to make the inference but just to clarify

zou3519 · 2021-02-25T22:39:27Z

aten/src/ATen/native/cuda/ForeachBinaryOpScalarList.cu

+
+// In the case of division, integer inputs will result in float. 
+// Currently multi tensor apply can only return result of the same type as input.
+void foreach_tensor_div_scalarlist_kernel_cuda_(TensorList tensors, at::ArrayRef<Scalar> scalars) {


Also nit: is it possible to update FOREACH_BINARY_OP_SCALARLIST to take three arguments, where the last argument is div_op? Then we would be able to use FOREACH_BINARY_OP_SCALARLIST with division instead of rewriting the division logic

zou3519 · 2021-02-25T22:39:43Z

aten/src/ATen/native/cuda/ForeachBinaryOpScalarList.cu

+    return foreach_binary_op<std::divides>(tensors, scalars);
+}
+
+// In the case of subtraction, we dont allow scalar to be boolean following the torch.sub logic


It would be good to mention "This does not use FOREACH_BINARY_OP_SCALARLIST because ". It's easy to make the inference but just to clarify that this is a special case

zou3519 · 2021-02-25T22:42:25Z

test/test_foreach.py

-                                foreach_bin_op_(tensors, scalars)
+                    with self.assertRaisesRegex(RuntimeError, "not implemented for"):
+                        foreach_bin_op(tensors, scalars)
+                    return


Should this be a continue?

zou3519 · 2021-02-25T22:44:14Z

test/test_foreach.py

+                scalars = [True for _ in range(N)]
+                scalars[0] = 1
+                scalars[1] = 1.1
+                scalars[2] = 3 + 5j


nit: can be done succinctly in one line: scalars = [1, 1.1, 3 + 5j] + [True for _ in range(N - 3)]

zou3519 · 2021-02-25T22:46:24Z

test/test_foreach.py

+                expected = [torch_bin_op(t, s) for t, s in zip(tensors, scalars)]
+                res = foreach_bin_op(tensors, scalars)
+                self.assertEqual(expected, res)


This only tests correctness for the out-of-place variant, right? Should it also test correctness for the in-place variant?

Differential Revision: [D25502939](https://our.internmc.facebook.com/intern/diff/D25502939) -------- Update logic for a set of foreach APIs with a scalar list - Update binary ops to support boolean type - Update sub ops to throw an error in case of bool subtraction - Update division op to check for type promotion. - Update tests [ghstack-poisoned]

zou3519 · 2021-03-11T20:31:56Z

test/test_foreach.py

+                else:
+                    with self.assertRaisesRegex(RuntimeError, "can't be cast to the desired output type"):
+                        foreach_bin_op_(tensors, scalars)
+                    continue


Is the continue necessary here?

Differential Revision: [D25502939](https://our.internmc.facebook.com/intern/diff/D25502939) -------- Update logic for a set of foreach APIs with a scalar list - Update binary ops to support boolean type - Update sub ops to throw an error in case of bool subtraction - Update division op to check for type promotion. - Update tests [ghstack-poisoned]

facebook-github-bot · 2021-03-15T19:17:55Z

@izdeby merged this pull request in 4bb34c2.

Update Binary Ops with scalar lists

20e274d

[ghstack-poisoned]

facebook-github-bot added the cla signed label Dec 11, 2020

izdeby requested review from cpuhrsch, gchanan, ngimel and zou3519 December 11, 2020 19:58

izdeby changed the title ~~Update Binary Ops with scalar lists~~ Update foreach binary ops with scalar lists Dec 11, 2020

Iurii Zdebskyi added 4 commits December 11, 2020 12:31

izdeby changed the title ~~Update foreach binary ops with scalar lists~~ Update ForeachBinaryOpScalarList.cu Dec 15, 2020

Iurii Zdebskyi added 7 commits December 15, 2020 15:08

zou3519 reviewed Dec 18, 2020

View reviewed changes

izdeby changed the title ~~Update ForeachBinaryOpScalarList.cu~~ Update ForeachBinaryOpScalarList.cu and refactor ForeachUtils.h Dec 21, 2020

izdeby requested a review from zou3519 December 21, 2020 19:32

This was referenced Feb 8, 2021

Update foreach APIs to use scalar lists #51893

Closed

Refactor ForeachUnaryOp.cu #51894

Closed

This was referenced Feb 18, 2021

[WIP] Support broadcasting via slow path #52448

Closed

[WIP] Add type promotion to a fast path for foreach APIs #52449

Closed

wip #52450

Closed

Iurii Zdebskyi added 2 commits February 18, 2021 12:15

izdeby requested a review from zou3519 February 19, 2021 16:03

zou3519 reviewed Feb 25, 2021

View reviewed changes

Iurii Zdebskyi and others added 6 commits March 1, 2021 14:58

izdeby requested a review from zou3519 March 9, 2021 22:54

zou3519 reviewed Mar 11, 2021

View reviewed changes

zou3519 approved these changes Mar 11, 2021

View reviewed changes

izdeby added 3 commits March 12, 2021 10:56

facebook-github-bot closed this in 4bb34c2 Mar 15, 2021

facebook-github-bot added the Merged label Mar 15, 2021

facebook-github-bot deleted the gh/izdeby/71/head branch March 19, 2021 14:16

		// In the case of division, integer inputs will result in float.
		// Currently multi tensor apply can only return result of the same type as input.

Conversation

izdeby commented Dec 11, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zou3519 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

izdeby commented Feb 18, 2021

Uh oh!

zou3519 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

facebook-github-bot commented Mar 15, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

izdeby commented Dec 11, 2020 •

edited

Loading