[fix] ReduceOps throw error if dim is repeated #44281

kshitij12345 · 2020-09-07T15:11:36Z

Fixes #44273

TODO

Add test

vadimkantorov · 2020-09-07T15:45:19Z

Should this be made into some reusable helper? So it's more discoverable if a similar check is decided for other ops

dr-ci · 2020-09-07T15:57:05Z

💊 CI failures summary and remediations

As of commit 5f4c242 (more details on the Dr. CI page):

1/1 failures possibly* introduced in this PR
- 1/1 non-CircleCI failure(s)

ci.pytorch.org: 1 failed

Failed: pr/pytorch-linux-bionic-rocm3.7-py3.6

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 on the GitHub issue tracker or post in the (internal) Dr. CI Users group.

See how this bot performed.

This comment has been revised 27 times.

kshitij12345 · 2020-09-07T16:27:52Z

Should this be made into some reusable helper? So it's more discoverable if a similar check is decided for other ops

Makes sense.

ezyang · 2020-09-08T02:41:17Z

aten/src/ATen/native/ReduceOps.cpp

@@ -358,6 +358,9 @@ Tensor& prod_out(Tensor& result, const Tensor& self, Dimname dim,

 Tensor &mean_out_cpu_gpu(Tensor &result, const Tensor &self, IntArrayRef dim,
                 bool keepdim, c10::optional<ScalarType> opt_dtype) {
+  auto dim_set = std::set<IntArrayRef::value_type>(dim.cbegin(), dim.cend());


This is probably not actually the most efficient way to do the duplicate check, since it involves doing a fairly unnecessary dynamic allocation for the set. Probably quickest when number of dims is small (which it should be usually) is just the quadratic nested loops version.

Makes sense.
~~What I had in mind was something like~~

if (dim.size() < 10) { // 10 is just some heurestic value // for loop version } else { // set version. }

~~Let me know if it sounds good and if you approve it what should the heuristic value be?~~

Thank You!

ezyang · 2020-09-08T02:47:37Z

test/test_torch.py

+    def test_mean_repeated_dim(self, device):
+        x = torch.randn(3, 3, 3, 3, device=device)
+        with self.assertRaisesRegex(RuntimeError, r'mean: repeated dimension in `dim` \(\[0, 0\]\)'):
+            torch.mean(x, dim=(0, 0))


@mruberry So, as was mentioned in the original issue, there are a bunch of operators which take in a list of dimensions. It seems like it would be useful to easily run a version of this test for all of the operators that do this :>

Good idea. I've made a note.

I had quickly searched the file below
https://github.com/pytorch/pytorch/blob/master/aten/src/ATen/native/native_functions.yaml
for int[1] based on the signature of

pytorch/aten/src/ATen/native/native_functions.yaml

Line 2093 in cce5982

- func: mean.dim(Tensor self, int[1] dim, bool keepdim=False, *, ScalarType? dtype=None) -> Tensor

facebook-github-bot

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

codecov · 2020-09-08T14:48:52Z

Codecov Report

Merging #44281 into master will increase coverage by 0.00%.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master   #44281   +/-   ##
=======================================
  Coverage   69.24%   69.24%           
=======================================
  Files         381      381           
  Lines       47573    47573           
=======================================
+ Hits        32942    32944    +2     
+ Misses      14631    14629    -2

Impacted Files	Coverage Δ
torch/utils/_benchmark/utils/common.py	`78.99% <0.00%> (+1.68%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 626e410...5f4c242. Read the comment docs.

* test more ops. * test repeated negative dim.

facebook-github-bot

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

ezyang · 2020-09-09T15:19:15Z

Ooh, this new version is much better, thanks!

facebook-github-bot

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

ezyang · 2020-09-09T17:45:39Z

cc @ailzhang on the xla bit

kshitij12345 · 2020-09-11T09:08:46Z

Gentle Ping :)

facebook-github-bot · 2020-09-12T02:15:46Z

@ezyang merged this pull request in 42f9f2f.

Summary: Fixes #44273 TODO * [x] Add test Pull Request resolved: #44281 Reviewed By: zhangguanheng66 Differential Revision: D23569004 Pulled By: ezyang fbshipit-source-id: 1ca6523fef168c8ce252aeb7ca418be346b297bf

fix: throw error if dim is repeated

045dea4

pytorchbot added the open source label Sep 7, 2020

add test

f4c29db

zhangguanheng66 added module: operators triaged This issue has been looked at a team member, and triaged and prioritized into an appropriate module labels Sep 7, 2020

zhangguanheng66 requested a review from ezyang September 7, 2020 20:22

ezyang reviewed Sep 8, 2020

View reviewed changes

ezyang approved these changes Sep 8, 2020

View reviewed changes

facebook-github-bot reviewed Sep 8, 2020

View reviewed changes

kshitij12345 added 5 commits September 8, 2020 20:28

use nested for-loop for checking unique dims

734283e

use at::dim_list_to_bitset

b8741e2

update test

9a771bf

* test more ops. * test repeated negative dim.

update test_dim_reduction_less_than_64

fc72433

add more ops

3ab4727

kshitij12345 changed the title ~~[fix] torch.mean throw error if dim is repeated~~ [fix] ReduceOps throw error if dim is repeated Sep 9, 2020

facebook-github-bot reviewed Sep 9, 2020

View reviewed changes

disable test on XLA as it doesn't raise Runtime Error

5f4c242

facebook-github-bot reviewed Sep 9, 2020

View reviewed changes

ezyang added the module: xla Related to XLA support label Sep 9, 2020

facebook-github-bot closed this in 42f9f2f Sep 11, 2020

facebook-github-bot added the merged label Sep 12, 2020

mruberry added the Merged label Oct 28, 2020

[fix] ReduceOps throw error if dim is repeated #44281

[fix] ReduceOps throw error if dim is repeated #44281

Uh oh!

Conversation

kshitij12345 commented Sep 7, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vadimkantorov commented Sep 7, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dr-ci bot commented Sep 7, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

💊 CI failures summary and remediations

ci.pytorch.org: 1 failed

Uh oh!

kshitij12345 commented Sep 7, 2020

Uh oh!

ezyang Sep 8, 2020

Choose a reason for hiding this comment

Uh oh!

kshitij12345 Sep 8, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ezyang Sep 8, 2020

Choose a reason for hiding this comment

Uh oh!

mruberry Sep 8, 2020

Choose a reason for hiding this comment

Uh oh!

kshitij12345 Sep 8, 2020

Choose a reason for hiding this comment

Uh oh!

facebook-github-bot left a comment

Choose a reason for hiding this comment

Uh oh!

codecov bot commented Sep 8, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

facebook-github-bot left a comment

Choose a reason for hiding this comment

Uh oh!

ezyang commented Sep 9, 2020

Uh oh!

facebook-github-bot left a comment

Choose a reason for hiding this comment

Uh oh!

ezyang commented Sep 9, 2020

Uh oh!

kshitij12345 commented Sep 11, 2020

Uh oh!

facebook-github-bot commented Sep 12, 2020

Uh oh!

Uh oh!

kshitij12345 commented Sep 7, 2020 •

edited

Loading

vadimkantorov commented Sep 7, 2020 •

edited

Loading

dr-ci bot commented Sep 7, 2020 •

edited

Loading

kshitij12345 Sep 8, 2020 •

edited

Loading

codecov bot commented Sep 8, 2020 •

edited

Loading