Fix `mean` bug for integral tensors. #76584

ysiraichi · 2022-04-29T10:05:22Z

The Problem: opt_dtype wasn't being taken into consideration when checking whether the input dtype was either floating point or complex dtype.

The Solution: run those checks with the dtype returned by get_dtype_from_self(self, opt_dtype, true).

This fix restores the original behavior, before #61643. It also improves the error message so that the user better comprehends what happened. Finally, I also added 2 tests for ensuring the issue was fixed

Before

>>> a = torch.randint(0, 5, (5, 5), dtype=torch.int64)
>>> b = torch.tensor([], dtype=torch.float32)

>>> a.mean() # no dtype
RuntimeError: mean(): input dtype should be either floating point or complex dtypes. Got Long instead.

>>> a.mean(dtype=torch.float32) # with dtype
RuntimeError: mean(): input dtype should be either floating point or complex dtypes. Got Long instead.

>>> torch.mean(a, [], dtype=torch.float64, out=b) # with mismatching dtype and out dtype
RuntimeError: mean(): input dtype should be either floating point or complex dtypes. Got Long instead.

After

>>> a = torch.randint(0, 5, (5, 5), dtype=torch.int64)
>>> b = torch.tensor([], dtype=torch.float32)

>>> a.mean() # no dtype
RuntimeError: mean(): at least one of (i) the input dtype and (ii) the desired output dtype should be either floating point or complex. Got (i) Long and (ii) None instead.

>>> a.mean(dtype=torch.float32) # with dtype
tensor(1.6800)

>>> torch.mean(a, [], dtype=torch.float64, out=b) # with mismatching dtype and out dtype
RuntimeError: Expected out tensor to have dtype double, but got float instead

facebook-github-bot · 2022-04-29T10:05:28Z

🔗 Helpful links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/76584
📄 Preview Python docs built from this PR
📄 Preview C++ docs built from this PR
❓Need help or want to give feedback on the CI? Visit our office hours

✅ No Failures (0 Pending)

As of commit 9d8ae5f (more details on the Dr. CI page):

Expand to see more

💚 💚 Looks good so far! There are no failures yet. 💚 💚

This comment was automatically generated by Dr. CI (expand for details).

Please report bugs/suggestions to the (internal) Dr. CI Users group.

Click here to manually regenerate this comment.

ysiraichi · 2022-04-29T10:07:52Z

@ngimel suggested casting the input directly to a floating point tensor. But, since I wasn't sure what the output dtype should be (half, float, or double), I simply fixed it so as to match the old behavior.

vadimkantorov · 2022-04-29T13:00:35Z

about auto-casting integral tensor (and fixing #64897): I propose to auto-cast to torch.get_default_dtype()

For practical use-cases: computing accuracy by taking mean of booltensor and computing a mean value of [0, 255] uint8 images etc. For them upcasting to float32 might be sufficient

mruberry · 2022-04-30T04:49:58Z

aten/src/ATen/native/ReduceOps.cpp

Probably want to hide this computation in a conditional that checks the error conditions so we're not creating this string on every call

mruberry · 2022-04-30T04:50:42Z

test/test_reductions.py

I think we can model this as an ErrorInput for the mean OpInfo?

mruberry · 2022-04-30T04:50:55Z

test/test_reductions.py

Same thing -- can we make this an ErrorInput?

ysiraichi · 2022-05-04T10:53:30Z

about auto-casting integral tensor (and fixing #64897): I propose to auto-cast to torch.get_default_dtype()

I think that makes sense to me, and hopefully it is not too confusing.
Given torch.mean(t, [], dtype=opt_dtype):

`t`	`opt_dtype`	Output dtype
integral tensor	`None`	`torch.get_default_dtype()`
*	floating or complex dtype	`opt_dtype`
floating or complex tensor	`None`	`t.dtype`

@mruberry @ngimel
What do you think?
That said, I think it is better to leave it to a future PR.

mruberry · 2022-05-04T10:57:02Z

about auto-casting integral tensor (and fixing #64897): I propose to auto-cast to torch.get_default_dtype()

I think that makes sense to me, and hopefully it is not too confusing. Given torch.mean(t, [], dtype=opt_dtype):

t opt_dtype Output dtype
integral tensor None torch.get_default_dtype()

floating or complex dtype opt_dtype
floating or complex tensor None t.dtype
@mruberry @ngimel What do you think? That said, I think it is better to leave it to a future PR.

That looks correct to me, but I'll let @ngimel review this more thoroughly since she's been working closely with reductions recently

vadimkantorov · 2022-05-04T13:47:02Z

For integral tensors one funny option may also be to do sum in int64 (or int32?) and then divide by the size - but maybe this might actually be slower and have worse dynamic range behavior than float

ysiraichi · 2022-05-07T13:47:50Z

@mruberry @ngimel
This is a friendly reminder. Do you have some time to take a look at this PR?
I think it is better to introduce the changes for supporting integral tensors in a separate PR (that's why I didn't implement them here).

ngimel · 2022-05-08T22:59:18Z

aten/src/ATen/native/ReduceOps.cpp

this is a misleading error message, e.g. if self dtype if float but dtype arg is integer, then it's and invalid combination (and you'll rightly error out), but error message wouldn't explain it.

ysiraichi · 2022-05-13T09:12:36Z

@mruberry @ngimel
This is a friendly reminder. Do you have some time to take a look at this PR?

ngimel · 2022-05-15T06:47:12Z

@pytorchbot merge this

github-actions · 2022-05-15T06:52:14Z

Hey @ysiraichi.
You've committed this PR, but it does not have both a 'release notes: ...' and 'topics: ...' label. Please add one of each to the PR. The 'release notes: ...' label should represent the part of PyTorch that this PR changes (fx, autograd, distributed, etc) and the 'topics: ...' label should represent the kind of PR it is (not user facing, new feature, bug fix, perf improvement, etc). The list of valid labels can be found here for the 'release notes: ...' and here for the 'topics: ...'.
For changes that are 'topic: not user facing' there is no need for a release notes label.

Summary: Fixes #76430 **The Problem:** `opt_dtype` wasn't being taken into consideration when checking whether the input dtype was either floating point or complex dtype. **The Solution:** run those checks with the dtype returned by `get_dtype_from_self(self, opt_dtype, true)`. This fix restores the original behavior, before #61643. It also improves the error message so that the user better comprehends what happened. Finally, I also added 2 tests for ensuring the issue was fixed ----- #### Before ```python >>> a = torch.randint(0, 5, (5, 5), dtype=torch.int64) >>> b = torch.tensor([], dtype=torch.float32) >>> a.mean() # no dtype RuntimeError: mean(): input dtype should be either floating point or complex dtypes. Got Long instead. >>> a.mean(dtype=torch.float32) # with dtype RuntimeError: mean(): input dtype should be either floating point or complex dtypes. Got Long instead. >>> torch.mean(a, [], dtype=torch.float64, out=b) # with mismatching dtype and out dtype RuntimeError: mean(): input dtype should be either floating point or complex dtypes. Got Long instead. ``` #### After ```python >>> a = torch.randint(0, 5, (5, 5), dtype=torch.int64) >>> b = torch.tensor([], dtype=torch.float32) >>> a.mean() # no dtype RuntimeError: mean(): at least one of (i) the input dtype and (ii) the desired output dtype should be either floating point or complex. Got (i) Long and (ii) None instead. >>> a.mean(dtype=torch.float32) # with dtype tensor(1.6800) >>> torch.mean(a, [], dtype=torch.float64, out=b) # with mismatching dtype and out dtype RuntimeError: Expected out tensor to have dtype double, but got float instead ``` Pull Request resolved: #76584 Approved by: https://github.com/ngimel Test Plan: contbuild & OSS CI, see https://hud.pytorch.org/commit/pytorch/pytorch/459e10f4465fd5a75e6dea96597b427feae71e42 Reviewed By: atalman Differential Revision: D36412508 Pulled By: atalman fbshipit-source-id: 7acfec825bc3efaaf62e124ddc24a1034fc6792e

ysiraichi requested review from mruberry and ngimel as code owners April 29, 2022 10:05

facebook-github-bot added the cla signed label Apr 29, 2022

pytorchbot added the open source label Apr 29, 2022

mruberry reviewed Apr 30, 2022

View reviewed changes

test/test_reductions.py Outdated

Copy link

Collaborator

mruberry Apr 30, 2022

Choose a reason for hiding this comment

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

Same thing -- can we make this an ErrorInput?

ysiraichi reacted with thumbs up emoji

mruberry added the triaged This issue has been looked at a team member, and triaged and prioritized into an appropriate module label May 3, 2022

ngimel reviewed May 8, 2022

View reviewed changes

ysiraichi added 8 commits May 11, 2022 19:19

Take into consideration the optional dtype for checking.

b6cd712

Add tests.

54ee14d

Fix optional type printing.

c8269e5

Split test into 2 clearer tests.

f20e05c

Remove unused f-string.

1490c7b

Add condition and make tests into ErrorInput instances.

abff173

Fix bad OpInfo changes.

b4f84dc

Fix error message.

52a2d4f

ysiraichi force-pushed the fix-mean-regression branch from 76074f7 to 52a2d4f Compare May 11, 2022 10:20

Fix OpInfo definition for mean.

9d8ae5f

ngimel approved these changes May 15, 2022

View reviewed changes

pytorchmergebot added the Merged label May 15, 2022

pytorchmergebot closed this in 459e10f May 15, 2022

ysiraichi added release notes: cpp release notes category topic: bug fixes topic category labels May 16, 2022

Fix mean bug for integral tensors. #76584

Fix mean bug for integral tensors. #76584

Uh oh!

Conversation

ysiraichi commented Apr 29, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Before

After

Uh oh!

facebook-github-bot commented Apr 29, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🔗 Helpful links

✅ No Failures (0 Pending)

Uh oh!

ysiraichi commented Apr 29, 2022

Uh oh!

vadimkantorov commented Apr 29, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mruberry Apr 30, 2022

Choose a reason for hiding this comment

Uh oh!

mruberry Apr 30, 2022

Choose a reason for hiding this comment

Uh oh!

mruberry Apr 30, 2022

Choose a reason for hiding this comment

Uh oh!

ysiraichi commented May 4, 2022

Uh oh!

mruberry commented May 4, 2022

Uh oh!

vadimkantorov commented May 4, 2022

Uh oh!

ysiraichi commented May 7, 2022

Uh oh!

ngimel May 8, 2022

Choose a reason for hiding this comment

Uh oh!

ysiraichi commented May 13, 2022

Uh oh!

ngimel commented May 15, 2022

Uh oh!

github-actions bot commented May 15, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

Fix `mean` bug for integral tensors. #76584

Fix `mean` bug for integral tensors. #76584

ysiraichi commented Apr 29, 2022 •

edited

Loading

facebook-github-bot commented Apr 29, 2022 •

edited

Loading

vadimkantorov commented Apr 29, 2022 •

edited

Loading