Enable product for bool tensor #48637

Kiyosora · 2020-12-01T02:54:24Z

Fixes #48351

dr-ci · 2020-12-01T03:12:56Z

💊 CI failures summary and remediations

As of commit 21bd03a (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 to the (internal) Dr. CI Users group.

This comment has been revised 46 times.

codecov · 2020-12-03T05:50:21Z

Codecov Report

Merging #48637 (21bd03a) into master (e9d7d37) will decrease coverage by 0.00%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master   #48637      +/-   ##
==========================================
- Coverage   80.62%   80.62%   -0.01%     
==========================================
  Files        1875     1875              
  Lines      202769   202769              
==========================================
- Hits       163486   163479       -7     
- Misses      39283    39290       +7

mruberry · 2020-12-08T07:52:55Z

test/test_reductions.py

@@ -814,6 +814,17 @@ def test_prod(self, device, dtype):
        torch.prod(x, 1, out=res2)
        self.assertEqual(res1, res2)

+    def test_prod_bool(self, device):
+        tests = [
+            ([True, True], True),


Add an empty tensor input, too, and instead of comparing with expected create a NumPy array from the inputs and compare with the result of np.prod.

Addressed! Thanks for suggestion!

mruberry · 2020-12-08T07:54:16Z

test/test_reductions.py

+
+        for val, exp in tests:
+            tensor = torch.tensor(val, device=device)
+            self.assertEqual(torch.prod(tensor, dtype=torch.bool), torch.tensor(exp, device=device))


What happens if dtype isn't set? That is, what is the default behavior when a bool tensor is passed to prod?

In this case (a bool tensor passed to prod without dtype variate), an int64 type Tensor would be returned, which is consistent with the performance in NumPy.

>>> np.prod(np.array([True,True])) 1 >>> torch.prod(torch.tensor([True, True])) tensor(1) >>> np.prod(np.array([True,True])).dtype dtype('int64') >>> torch.prod(torch.tensor([True, True])).dtype torch.int64

The only difference is that for the input of an empty Tensor, the result type of PyTorch is float32, while for NumPy, it is float64. I think it's caused by the different default types between PyTorch and NumPy, so I guess it's just fine.

>>> np.prod(np.array([])) 1.0 >>> np.prod(np.array([])).dtype dtype('float64') >>> torch.prod(torch.tensor([])) tensor(1.) >>> torch.prod(torch.tensor([])).dtype torch.float32

If the returned dtype is int64 when a boolean tensor is given as input, which code path is exercised in the function for boolean tensors? Because won't iter.dtype() return intt64?

I think the iter.dtype() here would return int64. The code would finaly goes to prod_out_impl, and it would use get_dtype method to casting inputs from kBool type to kLong before the Iterator be created.

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

Lines 426 to 430 in a20d451

ScalarType src_type = self.scalar_type();

if (promote_integers && at::isIntegralType(src_type, /*includeBool=*/true)) {

return kLong;

}

return src_type;

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

Lines 489 to 492 in a20d451

static Tensor& prod_out_impl(Tensor& result, const Tensor& self, IntArrayRef dim,

bool keepdim, c10::optional<ScalarType> opt_dtype) {

ScalarType dtype = get_dtype(result, self, opt_dtype, true);

auto iter = make_reduction("prod", result, self, dim, keepdim, dtype);

That's my thinking, too. Would you verify? Also, if that's the case, then is this code ever used?

if (iter.dtype() == ScalarType::Bool) { using scalar_t = bool; binary_kernel_reduce_vec( iter, [=](scalar_t a, scalar_t b) -> scalar_t { return a && b; }, [=](Vec256<scalar_t> a, Vec256<scalar_t> b) { return a && b; }, /*identity=*/1);

Yep, As shown below, I verified it in my environment.

When setting dtype as torch.bool, it would using the code of a&&b , and in other cases (including the above-mentioned default int64 scene), the part of a*b would be used.

Such verification has been verified in both CPU and CUDA environments, and the performance of the two is consistent.

static void prod_kernel_impl(TensorIterator& iter) { // Workaround for the error: '*' in boolean context, suggest '&&' instead [-Werror=int-in-bool -context] if (iter.dtype() == ScalarType::Bool) { std::cout << "Using a && b" << std::endl; using scalar_t = bool; binary_kernel_reduce_vec( iter, [=](scalar_t a, scalar_t b) -> scalar_t { return a && b; }, [=](Vec256<scalar_t> a, Vec256<scalar_t> b) { return a && b; }, /*identity=*/1); } else { std::cout << "Using a * b" << std::endl; AT_DISPATCH_ALL_TYPES_AND_COMPLEX(iter.dtype(), "prod_cpu", [&] { binary_kernel_reduce_vec( iter, [=](scalar_t a, scalar_t b) -> scalar_t { return a * b; }, [=](Vec256 <scalar_t> a, Vec256 <scalar_t> b) { return a * b; }, /*identity=*/1); }); } }

>>> import torch >>> torch.prod(torch.tensor([True, True]), dtype=torch.bool) Using a && b tensor(True) >>> torch.prod(torch.tensor([True, True])) Using a * b tensor(1) >>> torch.prod(torch.tensor([True, True], device='cuda'), dtype=torch.bool) Using a && b tensor(True, device='cuda:0') >>> torch.prod(torch.tensor([True, True])) Using a * b tensor(1)

Thank you for the explanation, @Kiyosora, that's very helpful.

In the future we may want to become more clever with this dispatch so we can compute in bool when the input is bool, even if the output dtype is something else (like int64), but this seems OK for now.

I've made one suggestion about extending the test to validate both the default bool->int64 behavior is consistent with NumPy and the already tested bool->bool behavior is, too.

mruberry · 2020-12-08T07:54:34Z

Hey @Kiyosora! Thanks for submitting this PR. Just a couple questions for you.

Kiyosora · 2020-12-09T07:23:25Z

Hey @Kiyosora! Thanks for submitting this PR. Just a couple questions for you.

Thanks for reviewing this PR, @mruberry ! 😃
It's looks fine that the result is consistent with NumPy when a bool tensor pass to prod.
In addition, the test case also has a bit of improvement, please take a look in your convenience.

mruberry · 2020-12-15T06:11:14Z

test/test_reductions.py

@@ -814,6 +814,13 @@ def test_prod(self, device, dtype):
        torch.prod(x, 1, out=res2)
        self.assertEqual(res1, res2)

+    def test_prod_bool(self, device):
+        vals = [[True, True], [True, False], [False, False], []]
+        for val in vals:


Would you extend this test to verify bool->int64 consistent with NumPy, too?

Addressed! Thanks for your suggestion!

mruberry

Cool! Thanks @Kiyosora!

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-12-21T23:13:35Z

@mruberry merged this pull request in 983bfc7.

Summary: Fixes pytorch#48351 Pull Request resolved: pytorch#48637 Reviewed By: mrshenli Differential Revision: D25658596 Pulled By: mruberry fbshipit-source-id: ff3ada74b6d281c8e4753ed38339a1c036f722ee

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

pytorchbot added the open source label Dec 1, 2020

Kiyosora force-pushed the product_for_bool_tensor branch 5 times, most recently from 7e3937f to 220e51d Compare December 2, 2020 08:11

Kiyosora changed the title ~~[WIP] Enable product for bool tensor~~ Enable product for bool tensor Dec 2, 2020

Kiyosora marked this pull request as ready for review December 2, 2020 08:37

bdhirsh requested a review from mruberry December 2, 2020 14:17

Kiyosora force-pushed the product_for_bool_tensor branch from 220e51d to f4d2d18 Compare December 3, 2020 02:24

heitorschueroff added module: boolean tensor triaged This issue has been looked at a team member, and triaged and prioritized into an appropriate module labels Dec 3, 2020

mruberry reviewed Dec 8, 2020

View reviewed changes

Kiyosora force-pushed the product_for_bool_tensor branch 2 times, most recently from 425e462 to dbadd20 Compare December 9, 2020 06:16

Kiyosora requested a review from mruberry December 9, 2020 07:04

mruberry reviewed Dec 15, 2020

View reviewed changes

Kiyosora force-pushed the product_for_bool_tensor branch 2 times, most recently from 233dbab to 65ad982 Compare December 15, 2020 08:13

Kiyosora added 4 commits December 16, 2020 11:04

Make bool tensor support in torch.prod

698ee2d

Add test case for bool tensor production

2dcd413

Fix Werror of int-in-bool-context

0063b5a

Improve test case

db1a49a

Kiyosora force-pushed the product_for_bool_tensor branch from f0aae22 to f55f363 Compare December 16, 2020 03:06

extend test case to verify bool -> int64

21bd03a

Kiyosora force-pushed the product_for_bool_tensor branch from f55f363 to 21bd03a Compare December 16, 2020 06:14

Kiyosora requested a review from mruberry December 16, 2020 08:56

mruberry approved these changes Dec 20, 2020

View reviewed changes

facebook-github-bot reviewed Dec 20, 2020

View reviewed changes

facebook-github-bot closed this in 983bfc7 Dec 21, 2020

facebook-github-bot added the Merged label Dec 21, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable product for bool tensor #48637

Enable product for bool tensor #48637

Kiyosora commented Dec 1, 2020

dr-ci bot commented Dec 1, 2020 •

edited by facebook-github-bot

codecov bot commented Dec 3, 2020 •

edited

mruberry Dec 8, 2020 •

edited

Kiyosora Dec 9, 2020

mruberry Dec 8, 2020

Kiyosora Dec 9, 2020

mruberry Dec 9, 2020

Kiyosora Dec 9, 2020

mruberry Dec 9, 2020

Kiyosora Dec 10, 2020

mruberry Dec 15, 2020

mruberry commented Dec 8, 2020

Kiyosora commented Dec 9, 2020

mruberry Dec 15, 2020

Kiyosora Dec 16, 2020 •

edited

mruberry left a comment

facebook-github-bot left a comment

facebook-github-bot commented Dec 21, 2020

	ScalarType src_type = self.scalar_type();
	if (promote_integers && at::isIntegralType(src_type, /includeBool=/true)) {
	return kLong;
	}
	return src_type;

	static Tensor& prod_out_impl(Tensor& result, const Tensor& self, IntArrayRef dim,
	bool keepdim, c10::optional<ScalarType> opt_dtype) {
	ScalarType dtype = get_dtype(result, self, opt_dtype, true);
	auto iter = make_reduction("prod", result, self, dim, keepdim, dtype);

Enable product for bool tensor #48637

Enable product for bool tensor #48637

Conversation

Kiyosora commented Dec 1, 2020

dr-ci bot commented Dec 1, 2020 • edited by facebook-github-bot

💊 CI failures summary and remediations

codecov bot commented Dec 3, 2020 • edited

Codecov Report

mruberry Dec 8, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mruberry commented Dec 8, 2020

Kiyosora commented Dec 9, 2020

Choose a reason for hiding this comment

Kiyosora Dec 16, 2020 • edited

Choose a reason for hiding this comment

mruberry left a comment

Choose a reason for hiding this comment

facebook-github-bot left a comment

Choose a reason for hiding this comment

facebook-github-bot commented Dec 21, 2020

dr-ci bot commented Dec 1, 2020 •

edited by facebook-github-bot

codecov bot commented Dec 3, 2020 •

edited

mruberry Dec 8, 2020 •

edited

Kiyosora Dec 16, 2020 •

edited