update upsample tests in test_nn.py to test for memory_format by bdhirsh · Pull Request #53665 · pytorch/pytorch

bdhirsh · 2021-03-09T23:31:22Z

@ngimel pointed out to me where we already test the behavior of the Upsample ops in test_nn.py. This PR deleting my bespoke tests in test_torch.py and updates those in test_nn.py to test memory format properly.

There were two reasons the original test didn't pick up on a memory format regression:

They didn't test the memory format of the output tensor explicitly, i.e. output.is_contiguous(memory_format=...)
Even with that change, the test tensors were to simple to fail the tests. From some trial and error, it looks like one of the first two dimensions in the inputs needs to be > 1 in order for the channels_last memory format to actually re-order the strides.

Stack from ghstack:

update upsample tests in test_nn.py to test for memory_format #53665 update upsample tests in test_nn.py to test for memory_format
fix channels_last bug in upsample kernels #53535 fix channels_last bug in upsample kernels

Differential Revision: D26929683

[ghstack-poisoned]

facebook-github-bot · 2021-03-09T23:31:45Z

💊 CI failures summary and remediations

As of commit 981e490 (more details on the Dr. CI page):

2/2 failures possibly* introduced in this PR
- 2/2 non-scanned failure(s)

ci.pytorch.org: 1 failed

Failed: pr/pytorch-linux-bionic-rocm4.0.1-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 to the (internal) Dr. CI Users group.

bdhirsh · 2021-03-09T23:36:50Z

-                with warnings.catch_warnings(record=True) as w:
-                    out_t = m(in_t)
-                self.assertEqual(torch.ones(1, 1, out_size, out_size), out_t.data)
+            for memory_format in [torch.contiguous_format, torch.channels_last]:


GitHub displays this with a lot of red and green, but all I really did was add a for loop here to loop over the different memory formats. Then further down I assert that the output is contiguous w.r.t the expected memory format.

@ngimel

…mat" @ngimel pointed out to me where we already test the behavior of the `Upsample` ops in `test_nn.py`. This PR deleting my bespoke tests in `test_torch.py` and updates those in `test_nn.py` to test memory format properly. There were two reasons the original test didn't pick up on a memory format regression: - They didn't test the memory format of the output tensor explicitly, i.e. `output.is_contiguous(memory_format=...)` - Even with that change, the test tensors were to simple to fail the tests. From some trial and error, it looks like one of the first two dimensions in the inputs needs to be > 1 in order for the `channels_last` memory format to actually re-order the strides. Differential Revision: [D26929683](https://our.internmc.facebook.com/intern/diff/D26929683) [ghstack-poisoned]

ghstack-source-id: 7bd885b Pull Request resolved: #53665

ngimel

Thanks! Please get rid of .data attributes, otherwise looks good.

ngimel · 2021-03-10T07:26:57Z

                out_uint8_t = m(in_uint8_t)
-            self.assertEqual(torch.ones(1, 1, 4, 4).contiguous(memory_format=memory_format), out_t.data)
-            self.assertEqual(torch.ones(1, 1, 4, 4, dtype=torch.uint8).contiguous(memory_format=memory_format), out_uint8_t.data)
+            self.assertEqual(torch.ones(1, 2, 4, 4).contiguous(memory_format=memory_format), out_t.data)


nit: .data is deprecated API, and you don't need to call .contiguous() on the expected tensor, so

self.assertEqual(torch.ones(...), out_t)

ngimel · 2021-03-10T07:27:16Z

-            self.assertEqual(torch.ones(1, 1, 4, 4).contiguous(memory_format=memory_format), out_t.data)
-            self.assertEqual(torch.ones(1, 1, 4, 4, dtype=torch.uint8).contiguous(memory_format=memory_format), out_uint8_t.data)
+            self.assertEqual(torch.ones(1, 2, 4, 4).contiguous(memory_format=memory_format), out_t.data)
+            self.assertEqual(torch.ones(1, 2, 4, 4, dtype=torch.uint8).contiguous(memory_format=memory_format), out_uint8_t.data)


same comment here

ngimel · 2021-03-10T07:27:33Z

+            self.assertEqual(torch.ones(1, 2, 4, 4).contiguous(memory_format=memory_format), out_t.data)
+            self.assertEqual(torch.ones(1, 2, 4, 4, dtype=torch.uint8).contiguous(memory_format=memory_format), out_uint8_t.data)
+            # Assert that memory format is carried through to the output
+            self.assertTrue(out_t.is_contiguous(memory_format=memory_format))


That's good!

ngimel · 2021-03-10T07:28:37Z

            with warnings.catch_warnings(record=True) as w:
                out_t = m(in_t)
-            self.assertEqual(torch.ones(1, 1, 4, 2).contiguous(memory_format=memory_format), out_t.data)
+            self.assertEqual(torch.ones(1, 2, 4, 2).contiguous(memory_format=memory_format), out_t.data)


probably makes sense to test memory_format here too?

@ngimel

…mat" @ngimel pointed out to me where we already test the behavior of the `Upsample` ops in `test_nn.py`. This PR deleting my bespoke tests in `test_torch.py` and updates those in `test_nn.py` to test memory format properly. There were two reasons the original test didn't pick up on a memory format regression: - They didn't test the memory format of the output tensor explicitly, i.e. `output.is_contiguous(memory_format=...)` - Even with that change, the test tensors were to simple to fail the tests. From some trial and error, it looks like one of the first two dimensions in the inputs needs to be > 1 in order for the `channels_last` memory format to actually re-order the strides. Differential Revision: [D26929683](https://our.internmc.facebook.com/intern/diff/D26929683) [ghstack-poisoned]

ghstack-source-id: 75f0e47 Pull Request resolved: #53665

ezyang · 2021-03-10T19:16:44Z

Thanks a lot for following up on this!

codecov · 2021-03-10T19:50:58Z

Codecov Report

Merging #53665 (981e490) into gh/bdhirsh/87/base (370bcc8) will increase coverage by 0.00%.
The diff coverage is n/a.

@@                 Coverage Diff                 @@
##           gh/bdhirsh/87/base   #53665   +/-   ##
===================================================
  Coverage               77.64%   77.65%           
===================================================
  Files                    1869     1869           
  Lines                  182291   182291           
===================================================
+ Hits                   141548   141549    +1     
+ Misses                  40743    40742    -1

facebook-github-bot · 2021-03-10T22:23:16Z

@bdhirsh merged this pull request in c68cc24.

…h#53665) Summary: Pull Request resolved: pytorch#53665 ngimel pointed out to me where we already test the behavior of the `Upsample` ops in `test_nn.py`. This PR deleting my bespoke tests in `test_torch.py` and updates those in `test_nn.py` to test memory format properly. There were two reasons the original test didn't pick up on a memory format regression: - They didn't test the memory format of the output tensor explicitly, i.e. `output.is_contiguous(memory_format=...)` - Even with that change, the test tensors were to simple to fail the tests. From some trial and error, it looks like one of the first two dimensions in the inputs needs to be > 1 in order for the `channels_last` memory format to actually re-order the strides. Test Plan: Imported from OSS Reviewed By: ngimel Differential Revision: D26929683 Pulled By: bdhirsh fbshipit-source-id: d17bc660ff031e9b3e2c93c60a9e9308e56ea612

update upsample tests in test_nn.py to test for memory_format

7270e83

[ghstack-poisoned]

facebook-github-bot added the cla signed label Mar 9, 2021

bdhirsh commented Mar 9, 2021

View reviewed changes

bdhirsh added a commit that referenced this pull request Mar 9, 2021

update upsample tests in test_nn.py to test for memory_format

dca55be

ghstack-source-id: 7bd885b Pull Request resolved: #53665

bdhirsh requested a review from ngimel March 9, 2021 23:45

ngimel approved these changes Mar 10, 2021

View reviewed changes

bdhirsh added a commit that referenced this pull request Mar 10, 2021

update upsample tests in test_nn.py to test for memory_format

87f1d36

ghstack-source-id: 75f0e47 Pull Request resolved: #53665

ngimel mentioned this pull request Mar 10, 2021

Improve memory format testing #53510

Open

facebook-github-bot closed this in c68cc24 Mar 10, 2021

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

facebook-github-bot deleted the gh/bdhirsh/87/head branch March 14, 2021 14:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

update upsample tests in test_nn.py to test for memory_format#53665

update upsample tests in test_nn.py to test for memory_format#53665
bdhirsh wants to merge 3 commits intogh/bdhirsh/87/basefrom
gh/bdhirsh/87/head

bdhirsh commented Mar 9, 2021 •

edited

Loading

Uh oh!

facebook-github-bot commented Mar 9, 2021 •

edited

Loading

Uh oh!

bdhirsh Mar 9, 2021

Uh oh!

ngimel left a comment

Uh oh!

ngimel Mar 10, 2021

Uh oh!

ngimel Mar 10, 2021

Uh oh!

ngimel Mar 10, 2021

Uh oh!

ngimel Mar 10, 2021

Uh oh!

ezyang commented Mar 10, 2021

Uh oh!

codecov Bot commented Mar 10, 2021

Uh oh!

facebook-github-bot commented Mar 10, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

bdhirsh commented Mar 9, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

facebook-github-bot commented Mar 9, 2021 • 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!

bdhirsh Mar 9, 2021

Choose a reason for hiding this comment

Uh oh!

ngimel left a comment

Choose a reason for hiding this comment

Uh oh!

ngimel Mar 10, 2021

Choose a reason for hiding this comment

Uh oh!

ngimel Mar 10, 2021

Choose a reason for hiding this comment

Uh oh!

ngimel Mar 10, 2021

Choose a reason for hiding this comment

Uh oh!

ngimel Mar 10, 2021

Choose a reason for hiding this comment

Uh oh!

ezyang commented Mar 10, 2021

Uh oh!

codecov Bot commented Mar 10, 2021

Codecov Report

Uh oh!

facebook-github-bot commented Mar 10, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

bdhirsh commented Mar 9, 2021 •

edited

Loading

facebook-github-bot commented Mar 9, 2021 •

edited

Loading