Implements volumetric (5d) affine grid generation. by elistevens · Pull Request #8322 · pytorch/pytorch

elistevens · 2018-06-10T21:35:19Z

I've implemented affine grid generation for volumetric (5d) inputs. The implementation is based off of the spatial implementation, extended by one dimension. I have a few questions about my implementation vs. the existing one that I will add inline.

I have some extensive test cases for the forward pass here: https://gist.github.com/elistevens/6e3bfb20d8d0652b83bd16b3e911285b However, they use pytest.fixture extensively, so I'm not sure the best way to incorporate them into the pytorch test suite. Suggestions? I have not tested backwards at all.

Diff probably best viewed with whitespace changes ignored.

Thanks for considering!

torch/nn/_functions/vision.py

+
+            base_grid[:, :, :, :, 0] = w_points
+            base_grid[:, :, :, :, 1] = h_points
+            base_grid[:, :, :, :, 2] = d_points


torch/nn/_functions/vision.py

-        base_grid[:, :, :, 1] = torch.ger(linear_points, torch.ones(W)).expand_as(base_grid[:, :, :, 1])
-        base_grid[:, :, :, 2] = 1
-        ctx.base_grid = base_grid
-        grid = torch.bmm(base_grid.view(N, H * W, 3), theta.transpose(1, 2))


torch/nn/_functions/vision.py

+            if theta.is_cuda:
+                AffineGridGenerator._enforce_cudnn(theta)
+                assert False
+            ctx.is_cuda = False


ssnl

Can you add tests for this too? And FYI this function should be eventually move to ATen c++ side one day, and the cudnn check logic will be a lot simpler when that happens.

torch/nn/_functions/vision.py

-        if grad_grid.is_cuda:
-            AffineGridGenerator._enforce_cudnn(grad_grid)
+        if len(ctx.size) == 5:
+            N, C, D, H, W = ctx.size


yf225 · 2018-07-10T16:49:34Z

@elistevens Let us know if you have time to address @ssnl 's comments

elistevens · 2018-07-10T19:54:17Z

Sorry for the slow response. I've been traveling and will be for another week still, but I intend to work on this when I get back.

…

On Tue, Jul 10, 2018, 09:50 Will Feng ***@***.***> wrote: @elistevens <https://github.com/elistevens> Let us know if you have time to address @ssnl <https://github.com/SsnL> 's comments — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#8322 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAIbIM1f9phgRQPh8qwHNDt2wMWsgDi6ks5uFNs4gaJpZM4Uh4b5> .

…ve checks on CuDNN etc.

elistevens · 2018-07-19T23:19:24Z

@ssnl @yf225 I've added tests and cleaned up the implementation.

Note that I've removed all of the "if theta.is_cuda then we must use CuDNN" checks. The affine_grid_generator does CuDNN dispatch, and if the control flow gets to AffineGridGenerator at all, it will happily work on CPU or CUDA tensors without complaint (while not implemented ATM, this would make it easier to verify that the python version outputs match the CuDNN outputs).

Are there any other changes desired?

elistevens · 2018-07-19T23:47:54Z

The jenkins builds are failing, though the tests work locally. There are going to be some extra pushes while I try and figure out what the difference is between my local setup and the formal testing. Sorry for the noise.

elistevens · 2018-07-23T16:10:13Z

@soumith Yes, absolutely. This was intended to be a temporary measure to get around the older version issue. I'll revert it once the docker images are updated. :)

yf225 · 2018-07-23T19:16:03Z

@elistevens I just updated the scipy version in our docker images to 1.1.0. Do you mind reverting the change to requirements.txt and see if CI passes?

elistevens · 2018-07-23T19:46:36Z

Will do tonight. Thanks!

…

On Mon, Jul 23, 2018, 12:16 Will Feng ***@***.***> wrote: @elistevens <https://github.com/elistevens> I just updated the scipy version in our docker images to 1.1.0. Do you mind reverting the change to requirements.txt and see if CI passes? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#8322 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAIbIMTkQVNulr40F5hSHuhxYXRYCk9Cks5uJiEKgaJpZM4Uh4b5> .

elistevens · 2018-07-24T00:21:40Z

In case anyone is wondering about the RuntimeWarning: numpy.dtype size changed, may indicate binary incompatibility. Expected 96, got 88 warnings, they're not actually a problem according to: https://stackoverflow.com/questions/40845304/runtimewarning-numpy-dtype-size-changed-may-indicate-binary-incompatibility

https://ci.pytorch.org/jenkins/job/pytorch-builds/job/pytorch-linux-trusty-pynightly-test/11964/console doesn't have scipy installed.

https://ci.pytorch.org/jenkins/job/pytorch-builds/job/pytorch-win-ws2016-cuda9-cudnn7-py3-build/13558/console

23:14:25    Creating library build\temp.win-amd64-3.6\Release\torch/csrc\_C.cp36-win_amd64.lib and object build\temp.win-amd64-3.6\Release\torch/csrc\_C.cp36-win_amd64.exp
23:14:25 Size.obj : error LNK2001: unresolved external symbol "struct torch::jit::Value * __cdecl torch::jit::insertConstant(struct torch::jit::Graph &,struct torch::jit::IValue,class at::optional<struct torch::jit::script::SourceRange>)" (?insertConstant@jit@torch@@YAPEAUValue@12@AEAUGraph@12@UIValue@12@V?$optional@USourceRange@script@jit@torch@@@at@@@Z)
23:14:25 build\lib.win-amd64-3.6\torch\_C.cp36-win_amd64.pyd : fatal error LNK1120: 1 unresolved externals
23:14:25 error: command 'C:\\Program Files (x86)\\Microsoft Visual Studio\\2017\\Community\\VC\\Tools\\MSVC\\14.11.25503\\bin\\HostX86\\x64\\link.exe' failed with exit status 1120

This doesn't seem to be related to my branch. Will this test failing be a merge blocker?

I think the rest of the issues are due to integer division under python 2. Testing now.

elistevens · 2018-07-26T23:34:21Z

The last build had a handful of errors that all seemed unrelated to my changes (missing scipy on some of the testing images, windows build error, etc.).

@ssnl Are there any other desired changes?

ezyang · 2018-07-27T04:32:28Z

@pytorchbot retest this please

elistevens · 2018-07-30T23:17:28Z

@yf225 the remaining three failures are all due to scipy being missing entirely from the relevant images. Is that something you care to fix?

yf225 · 2018-07-30T23:25:21Z

@elistevens We should probably use the TEST_SCIPY flag (https://github.com/pytorch/pytorch/blob/master/test/common.py#L84) to gate the tests that require scipy in test_affine.py.

elistevens · 2018-07-31T16:24:14Z

What's the project policy on PRs with failing tests that are unrelated to the PR? Do I need all green before a merge will be considered?

I'm not sure if I should be merging master and hoping the issues get resolved, or if review of the failures is sufficient.

elistevens · 2018-08-13T17:26:06Z

@ssnl @yf225 any further changes desired?

test/test_affine.py

@@ -0,0 +1,602 @@
+from __future__ import print_function, division


soumith · 2018-08-14T17:01:34Z

@elistevens after Simon's comment is addressed, this PR is good to go, i.e.:

Can you fold this in test_nn and remove the comments/logging please?

test/run_test.py

@@ -1,345 +1,345 @@
-#!/usr/bin/env python


Keeps the 'affine' line removal, since that's still needed.

test/test_nn.py

+    return transform_tensor, transform_ary, grid_ary
+
+
+class TestAffine(TestCase):


facebook-github-bot

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

Summary: I've implemented affine grid generation for volumetric (5d) inputs. The implementation is based off of the spatial implementation, extended by one dimension. I have a few questions about my implementation vs. the existing one that I will add inline. I have some extensive test cases for the forward pass here: https://gist.github.com/elistevens/6e3bfb20d8d0652b83bd16b3e911285b However, they use `pytest.fixture` extensively, so I'm not sure the best way to incorporate them into the pytorch test suite. Suggestions? I have not tested backwards at all. Diff probably best viewed with whitespace changes ignored. Thanks for considering! Pull Request resolved: pytorch#8322 Differential Revision: D9332335 Pulled By: SsnL fbshipit-source-id: 1b3a91d078ef41a6d0a800514e49298fd817e4df

Summary: Pull Request resolved: #10775 Differential Revision: D9458207 Pulled By: SsnL fbshipit-source-id: f2b0dbf2d236134afded9b15d8bf55ff98f50e7b

Summary: Pull Request resolved: pytorch#10775 Differential Revision: D9458207 Pulled By: SsnL fbshipit-source-id: f2b0dbf2d236134afded9b15d8bf55ff98f50e7b

Implements volumetric (5d) affine grid generation.

dd64ad7

elistevens requested review from apaszke, colesbury, ezyang, gchanan, soumith and zdevito as code owners June 10, 2018 21:35

elistevens commented Jun 10, 2018

View reviewed changes

torch/nn/_functions/vision.py Outdated

base_grid[:, :, :, :, 0] = w_points

base_grid[:, :, :, :, 1] = h_points

base_grid[:, :, :, :, 2] = d_points

This comment was marked as off-topic.

Sign in to view

elistevens commented Jun 10, 2018

View reviewed changes

torch/nn/_functions/vision.py Outdated

if theta.is_cuda:

AffineGridGenerator._enforce_cudnn(theta)

assert False

ctx.is_cuda = False

This comment was marked as off-topic.

Sign in to view

ssnl requested changes Jun 26, 2018

View reviewed changes

elistevens added 2 commits July 19, 2018 15:51

Added affine tests. Test style is adapted from pytest fixtures.

43bde34

Clean up AffineGridGenerator implementation. Removes overly-restricti…

f1ea93e

…ve checks on CuDNN etc.

elistevens added 2 commits July 22, 2018 14:24

Fix lint errors, prints scipy version.

f809b1f

Update scipy version, more flake8 fixes.

1d109c6

elistevens added 2 commits July 23, 2018 16:02

Remove scipy requirement, replace @ with np.dot(...).

660fbb6

Adds py2 from future import division

db85356

Merge branch 'master' into affine_grid_volumetric_support

7cba5b6

Added TEST_SCIPY etc. guards.

cfd41aa

ssnl reviewed Aug 14, 2018

View reviewed changes

test/test_affine.py Outdated

@@ -0,0 +1,602 @@

from __future__ import print_function, division

This comment was marked as off-topic.

Sign in to view

li-roy added the awaiting response (this tag is deprecated) This tag is deprecated while we figure out what to do with it label Aug 14, 2018

Move TestAffine to test_nn.py

c600fbb

soumith reviewed Aug 14, 2018

View reviewed changes

test/run_test.py

@@ -1,345 +1,345 @@

#!/usr/bin/env python

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

elistevens added 3 commits August 14, 2018 12:16

Correct unintended whitespace change to run_test.py

c99639d

Keeps the 'affine' line removal, since that's still needed.

Merge branch 'master' into affine_grid_volumetric_support

16a5e38

Fixing line-too-long error.

76d61c7

ssnl reviewed Aug 15, 2018

View reviewed changes

test/test_nn.py Outdated

return transform_tensor, transform_ary, grid_ary

class TestAffine(TestCase):

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

Move affine tests into TestNN

b74a18b

ssnl approved these changes Aug 15, 2018

View reviewed changes

facebook-github-bot reviewed Aug 15, 2018

View reviewed changes

facebook-github-bot closed this in f5a4dd8 Aug 15, 2018

elistevens deleted the affine_grid_volumetric_support branch August 18, 2018 04:43

facebook-github-bot pushed a commit that referenced this pull request Aug 22, 2018

Resubmit #8322 with scipy version check

de11a5f

Summary: Pull Request resolved: #10775 Differential Revision: D9458207 Pulled By: SsnL fbshipit-source-id: f2b0dbf2d236134afded9b15d8bf55ff98f50e7b

ezyang added open source merged labels Jun 24, 2019

		@@ -0,0 +1,602 @@
		from __future__ import print_function, division

		return transform_tensor, transform_ary, grid_ary


		class TestAffine(TestCase):

Conversation

elistevens commented Jun 10, 2018

Uh oh!

This comment was marked as off-topic.

Uh oh!

This comment was marked as off-topic.

Uh oh!

This comment was marked as off-topic.

Uh oh!

ssnl left a comment

Choose a reason for hiding this comment

Uh oh!

This comment was marked as off-topic.

Uh oh!

This comment was marked as off-topic.

Uh oh!

This comment was marked as off-topic.

Uh oh!

yf225 commented Jul 10, 2018

Uh oh!

elistevens commented Jul 10, 2018 via email

Uh oh!

elistevens commented Jul 19, 2018

Uh oh!

elistevens commented Jul 19, 2018

Uh oh!

elistevens commented Jul 23, 2018

Uh oh!

yf225 commented Jul 23, 2018

Uh oh!

elistevens commented Jul 23, 2018 via email

Uh oh!

elistevens commented Jul 24, 2018

Uh oh!

elistevens commented Jul 26, 2018

Uh oh!

ezyang commented Jul 27, 2018

Uh oh!

elistevens commented Jul 30, 2018

Uh oh!

yf225 commented Jul 30, 2018

Uh oh!

elistevens commented Jul 31, 2018

Uh oh!

elistevens commented Aug 13, 2018

Uh oh!

This comment was marked as off-topic.

Uh oh!

soumith commented Aug 14, 2018

Uh oh!

This comment was marked as off-topic.

Uh oh!

This comment was marked as off-topic.

Uh oh!

This comment was marked as off-topic.

Uh oh!

This comment was marked as off-topic.

Uh oh!

This comment was marked as off-topic.

Uh oh!

facebook-github-bot left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants