GitHub PullRequestReviewCommentEvent at 2014-08-19 02:07:05
Repository: Theano
Actor: stencilman
Payload: {"action":"created","comment":{"url":"https://api.github.com/repos/Theano/Theano/pulls/comments/16393754","id":16393754,"diff_hunk":"@@ -1288,10 +1289,26 @@ def local_conv_gemm(node):\n     if (isinstance(node.op, GpuConv) and\n         node.op.border_mode in ['full', 'valid']):\n         img, kern = node.inputs\n-        img = gpu_contiguous(img)\n-        kern = kern[:, :, ::-1, ::-1]\n-        kern = gpu_contiguous(kern)\n-        return [GpuCorrMM(node.op.border_mode, node.op.subsample)(img, kern)]\n+        border_mode = node.op.border_mode\n+        subsample = node.op.subsample\n+        pad = (0,0)\n+        if (border_mode == 'full') and (subsample != (1,1)):\n+            # need to simulate this via a padded valid convolution\n+            pad = 'auto'\n+            border_mode = 'valid'\n+        if (border_mode == 'valid'):\n+            # need to flip the kernel for valid convolution\n+            kern = kern[:, :, ::-1, ::-1]\n+            # call GpuCorrMM\n+            # TODO: call GpuCorrMM_gradWeights instead if appropriate\n+            return [GpuCorrMM('valid', subsample, pad)(\n+                    gpu_contiguous(img), gpu_contiguous(kern))]","path":"theano/sandbox/cuda/opt.py","position":31,"original_position":31,"commit_id":"c2db8b1dfbff90046fc54d16f34f80307a324625","original_commit_id":"5699b27668b93e273795312758f86131b1c72bdd","user":{"login":"stencilman","id":4626592,"avatar_url":"https://avatars.githubusercontent.com/u/4626592?v=2","gravatar_id":"8a61ccb10b9f38c8955fb97d538b8221","url":"https://api.github.com/users/stencilman","html_url":"https://github.com/stencilman","followers_url":"https://api.github.com/users/stencilman/followers","following_url":"https://api.github.com/users/stencilman/following{/other_user}","gists_url":"https://api.github.com/users/stencilman/gists{/gist_id}","starred_url":"https://api.github.com/users/stencilman/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/stencilman/subscriptions","organizations_url":"https://api.github.com/users/stencilman/orgs","repos_url":"https://api.github.com/users/stencilman/repos","events_url":"https://api.github.com/users/stencilman/events{/privacy}","received_events_url":"https://api.github.com/users/stencilman/received_events","type":"User","site_admin":false},"body":"Yes, I will do this now (Time GpuCorrMM vs GpuCorr_gradWeights for different input size, including your examples).","created_at":"2014-08-19T02:07:05Z","updated_at":"2014-08-19T02:07:05Z","html_url":"https://github.com/Theano/Theano/pull/2033#discussion_r16393754","pull_request_url":"https://api.github.com/repos/Theano/Theano/pulls/2033","_links":{"self":{"href":"https://api.github.com/repos/Theano/Theano/pulls/comments/16393754"},"html":{"href":"https://github.com/Theano/Theano/pull/2033#discussion_r16393754"},"pull_request":{"href":"https://api.github.com/repos/Theano/Theano/pulls/2033"}}},"pull_request":{"url":"https://api.github.com/repos/Theano/Theano/pulls/2033","id":19679518,"html_url":"https://github.com/Theano/Theano/pull/2033","diff_url":"https://github.com/Theano/Theano/pull/2033.diff","patch_url":"https://github.com/Theano/Theano/pull/2033.patch","issue_url":"https://api.github.com/repos/Theano/Theano/issues/2033","number":2033,"state":"open","locked":false,"title":"Faster full convolution for GpuCorrMM","user":{"login":"f0k","id":629706,"avatar_url":"https://avatars.githubusercontent.com/u/629706?v=2","gravatar_id":"45a79db4270c6adceddf0cdc00a72281","url":"https://api.github.com/users/f0k","html_url":"https://github.com/f0k","followers_url":"https://api.github.com/users/f0k/followers","following_url":"https://api.github.com/users/f0k/following{/other_user}","gists_url":"https://api.github.com/users/f0k/gists{/gist_id}","starred_url":"https://api.github.com/users/f0k/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/f0k/subscriptions","organizations_url":"https://api.github.com/users/f0k/orgs","repos_url":"https://api.github.com/users/f0k/repos","events_url":"https://api.github.com/users/f0k/events{/privacy}","received_events_url":"https://api.github.com/users/f0k/received_events","type":"User","site_admin":false},"body":"As a follow-up to #2023, this PR adds [caffe's backward pass wrt. inputs](https://github.com/BVLC/caffe/blob/master/src/caffe/layers/conv_layer.cu#L98) to GpuCorrMM to implement `border_mode=\"full\"`. It passes all the tests and it's about 2-4x faster than simulating a full convolution with a padded valid convolution. On the way, it cleans up the code and adds some more elaborate documentation.\r\n\r\nThere are some caveats, though, all stemming from the fact that the implementation in caffe is meant as a backward pass for a valid correlation:\r\n* With `border_mode=\"valid\"`, `GpuCorrMM` doesn't flip the kernels, but with `border_mode=\"full\"`, it does.\r\n* With `border_mode=\"valid\"`, `subsampling` is for subsampling the output, but with `border_mode=\"full\"`, it is for upsampling the input.\r\n* With `border_mode=\"valid\"`, `pad` is for padding the input, but with `border_mode=\"full\"`, it is for cropping the output.\r\n* With `border_mode=\"full\"`, it needs a different memory layout for the kernels.\r\n\r\nCurrently, `GpuCorrMM` directly wraps the underlying algorithm, and `local_conv_gemm()` copes with the different peculiarities, because I wasn't sure whether the `GpuCorrMM` Op should be inserting dimshuffles, kernel flips and `gpu_contiguous`es on its own.\r\n\r\nLooking at the end result, although it's quite fast, `local_conv_gemm()` now basically undoes everything that `ConvOp.grad()` does. It might be possible to write an optimizer that replaces the gradient of a valid convolution with a properly parameterized `GpuCorrMM` Op, instead of just replacing the full convolution Op that is part of the gradient (introducing redundant dimshuffles etc. on the way). This way we would leverage the caffe implementation better.\r\nThe alternative would be to modify the CUDA code to perform a subsampled, padded, full correlation, as would be expected from a Gpu Correlation Op. This would be cleaner, but it would also mean a lot more work and we wouldn't profit as much from caffe's implementation for the case of `subsampling != (1,1)` or `padding != (0,0)` (granted, this may be an uninteresting case in practice anyway).\r\n/edit: Another alternative would be splitting this into two Ops: `GpuCorrMM` for valid correlation with padding and subsampling (the caffe forward pass), and `GpuConvMM` for full convolution with upsampling and cropping (the caffe backward pass). This way it would be obvious how the operations differ, but the memory layout required for the second Op would still be unintuitive.\r\nYet another alternative would be splitting it into `GpuCorrMM` and `GpuCorrMM_gradInput`. This way it would be obvious how to use `GpuCorrMM_gradInput` for computing the gradient of `GpuCorrMM`. We could even add `GpuCorrMM_gradKernel` for the gradient wrt. weights; it seems caffe does things slightly different there as well.\r\nIn both cases, `GpuCorrMM` should get a `grad()` method to define its gradient (so it can be used directly in a CNN to avoid kernel flipping and whatnot) and `local_conv_gemm()` should still be able to replace any `GpuConv` instances with a gemm-based Op (so it can be used with existing CNN implementations).","created_at":"2014-08-12T18:18:03Z","updated_at":"2014-08-19T02:07:05Z","closed_at":null,"merged_at":null,"merge_commit_sha":"b7d9833ba4607a7adf90e835101201ba283f7920","assignee":null,"milestone":null,"commits_url":"https://api.github.com/repos/Theano/Theano/pulls/2033/commits","review_comments_url":"https://api.github.com/repos/Theano/Theano/pulls/2033/comments","review_comment_url":"https://api.github.com/repos/Theano/Theano/pulls/comments/{number}","comments_url":"https://api.github.com/repos/Theano/Theano/issues/2033/comments","statuses_url":"https://api.github.com/repos/Theano/Theano/statuses/c2db8b1dfbff90046fc54d16f34f80307a324625","head":{"label":"f0k:corrmm-faster-fullconv","ref":"corrmm-faster-fullconv","sha":"c2db8b1dfbff90046fc54d16f34f80307a324625","user":{"login":"f0k","id":629706,"avatar_url":"https://avatars.githubusercontent.com/u/629706?v=2","gravatar_id":"45a79db4270c6adceddf0cdc00a72281","url":"https://api.github.com/users/f0k","html_url":"https://github.com/f0k","followers_url":"https://api.github.com/users/f0k/followers","following_url":"https://api.github.com/users/f0k/following{/other_user}","gists_url":"https://api.github.com/users/f0k/gists{/gist_id}","starred_url":"https://api.github.com/users/f0k/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/f0k/subscriptions","organizations_url":"https://api.github.com/users/f0k/orgs","repos_url":"https://api.github.com/users/f0k/repos","events_url":"https://api.github.com/users/f0k/events{/privacy}","received_events_url":"https://api.github.com/users/f0k/received_events","type":"User","site_admin":false},"repo":{"id":9244648,"name":"Theano","full_name":"f0k/Theano","owner":{"login":"f0k","id":629706,"avatar_url":"https://avatars.githubusercontent.com/u/629706?v=2","gravatar_id":"45a79db4270c6adceddf0cdc00a72281","url":"https://api.github.com/users/f0k","html_url":"https://github.com/f0k","followers_url":"https://api.github.com/users/f0k/followers","following_url":"https://api.github.com/users/f0k/following{/other_user}","gists_url":"https://api.github.com/users/f0k/gists{/gist_id}","starred_url":"https://api.github.com/users/f0k/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/f0k/subscriptions","organizations_url":"https://api.github.com/users/f0k/orgs","repos_url":"https://api.github.com/users/f0k/repos","events_url":"https://api.github.com/users/f0k/events{/privacy}","received_events_url":"https://api.github.com/users/f0k/received_events","type":"User","site_admin":false},"private":false,"html_url":"https://github.com/f0k/Theano","description":"Optimizing GPU-meta-programming code generating array oriented optimizing math compiler in Python","fork":true,"url":"https://api.github.com/repos/f0k/Theano","forks_url":"https://api.github.com/repos/f0k/Theano/forks","keys_url":"https://api.github.com/repos/f0k/Theano/keys{/key_id}","collaborators_url":"https://api.github.com/repos/f0k/Theano/collaborators{/collaborator}","teams_url":"https://api.github.com/repos/f0k/Theano/teams","hooks_url":"https://api.github.com/repos/f0k/Theano/hooks","issue_events_url":"https://api.github.com/repos/f0k/Theano/issues/events{/number}","events_url":"https://api.github.com/repos/f0k/Theano/events","assignees_url":"https://api.github.com/repos/f0k/Theano/assignees{/user}","branches_url":"https://api.github.com/repos/f0k/Theano/branches{/branch}","tags_url":"https://api.github.com/repos/f0k/Theano/tags","blobs_url":"https://api.github.com/repos/f0k/Theano/git/blobs{/sha}","git_tags_url":"https://api.github.com/repos/f0k/Theano/git/tags{/sha}","git_refs_url":"https://api.github.com/repos/f0k/Theano/git/refs{/sha}","trees_url":"https://api.github.com/repos/f0k/Theano/git/trees{/sha}","statuses_url":"https://api.github.com/repos/f0k/Theano/statuses/{sha}","languages_url":"https://api.github.com/repos/f0k/Theano/languages","stargazers_url":"https://api.github.com/repos/f0k/Theano/stargazers","contributors_url":"https://api.github.com/repos/f0k/Theano/contributors","subscribers_url":"https://api.github.com/repos/f0k/Theano/subscribers","subscription_url":"https://api.github.com/repos/f0k/Theano/subscription","commits_url":"https://api.github.com/repos/f0k/Theano/commits{/sha}","git_commits_url":"https://api.github.com/repos/f0k/Theano/git/commits{/sha}","comments_url":"https://api.github.com/repos/f0k/Theano/comments{/number}","issue_comment_url":"https://api.github.com/repos/f0k/Theano/issues/comments/{number}","contents_url":"https://api.github.com/repos/f0k/Theano/contents/{+path}","compare_url":"https://api.github.com/repos/f0k/Theano/compare/{base}...{head}","merges_url":"https://api.github.com/repos/f0k/Theano/merges","archive_url":"https://api.github.com/repos/f0k/Theano/{archive_format}{/ref}","downloads_url":"https://api.github.com/repos/f0k/Theano/downloads","issues_url":"https://api.github.com/repos/f0k/Theano/issues{/number}","pulls_url":"https://api.github.com/repos/f0k/Theano/pulls{/number}","milestones_url":"https://api.github.com/repos/f0k/Theano/milestones{/number}","notifications_url":"https://api.github.com/repos/f0k/Theano/notifications{?since,all,participating}","labels_url":"https://api.github.com/repos/f0k/Theano/labels{/name}","releases_url":"https://api.github.com/repos/f0k/Theano/releases{/id}","created_at":"2013-04-05T15:52:31Z","updated_at":"2014-06-06T16:11:50Z","pushed_at":"2014-08-18T19:20:13Z","git_url":"git://github.com/f0k/Theano.git","ssh_url":"git@github.com:f0k/Theano.git","clone_url":"https://github.com/f0k/Theano.git","svn_url":"https://github.com/f0k/Theano","homepage":"http://www.deeplearning.net/software/theano","size":20943,"stargazers_count":0,"watchers_count":0,"language":"Python","has_issues":false,"has_downloads":true,"has_wiki":true,"forks_count":0,"mirror_url":null,"open_issues_count":0,"forks":0,"open_issues":0,"watchers":0,"default_branch":"master"}},"base":{"label":"Theano:master","ref":"master","sha":"9b3ea9e0bfec1a8d17cc518329a9dcded9f442ab","user":{"login":"Theano","id":970517,"avatar_url":"https://avatars.githubusercontent.com/u/970517?v=2","gravatar_id":"f2c9cd1d69fe2fb37115161c5b1e1708","url":"https://api.github.com/users/Theano","html_url":"https://github.com/Theano","followers_url":"https://api.github.com/users/Theano/followers","following_url":"https://api.github.com/users/Theano/following{/other_user}","gists_url":"https://api.github.com/users/Theano/gists{/gist_id}","starred_url":"https://api.github.com/users/Theano/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/Theano/subscriptions","organizations_url":"https://api.github.com/users/Theano/orgs","repos_url":"https://api.github.com/users/Theano/repos","events_url":"https://api.github.com/users/Theano/events{/privacy}","received_events_url":"https://api.github.com/users/Theano/received_events","type":"Organization","site_admin":false},"repo":{"id":2183193,"name":"Theano","full_name":"Theano/Theano","owner":{"login":"Theano","id":970517,"avatar_url":"https://avatars.githubusercontent.com/u/970517?v=2","gravatar_id":"f2c9cd1d69fe2fb37115161c5b1e1708","url":"https://api.github.com/users/Theano","html_url":"https://github.com/Theano","followers_url":"https://api.github.com/users/Theano/followers","following_url":"https://api.github.com/users/Theano/following{/other_user}","gists_url":"https://api.github.com/users/Theano/gists{/gist_id}","starred_url":"https://api.github.com/users/Theano/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/Theano/subscriptions","organizations_url":"https://api.github.com/users/Theano/orgs","repos_url":"https://api.github.com/users/Theano/repos","events_url":"https://api.github.com/users/Theano/events{/privacy}","received_events_url":"https://api.github.com/users/Theano/received_events","type":"Organization","site_admin":false},"private":false,"html_url":"https://github.com/Theano/Theano","description":"Optimizing GPU-meta-programming code generating array oriented optimizing math compiler in Python","fork":false,"url":"https://api.github.com/repos/Theano/Theano","forks_url":"https://api.github.com/repos/Theano/Theano/forks","keys_url":"https://api.github.com/repos/Theano/Theano/keys{/key_id}","collaborators_url":"https://api.github.com/repos/Theano/Theano/collaborators{/collaborator}","teams_url":"https://api.github.com/repos/Theano/Theano/teams","hooks_url":"https://api.github.com/repos/Theano/Theano/hooks","issue_events_url":"https://api.github.com/repos/Theano/Theano/issues/events{/number}","events_url":"https://api.github.com/repos/Theano/Theano/events","assignees_url":"https://api.github.com/repos/Theano/Theano/assignees{/user}","branches_url":"https://api.github.com/repos/Theano/Theano/branches{/branch}","tags_url":"https://api.github.com/repos/Theano/Theano/tags","blobs_url":"https://api.github.com/repos/Theano/Theano/git/blobs{/sha}","git_tags_url":"https://api.github.com/repos/Theano/Theano/git/tags{/sha}","git_refs_url":"https://api.github.com/repos/Theano/Theano/git/refs{/sha}","trees_url":"https://api.github.com/repos/Theano/Theano/git/trees{/sha}","statuses_url":"https://api.github.com/repos/Theano/Theano/statuses/{sha}","languages_url":"https://api.github.com/repos/Theano/Theano/languages","stargazers_url":"https://api.github.com/repos/Theano/Theano/stargazers","contributors_url":"https://api.github.com/repos/Theano/Theano/contributors","subscribers_url":"https://api.github.com/repos/Theano/Theano/subscribers","subscription_url":"https://api.github.com/repos/Theano/Theano/subscription","commits_url":"https://api.github.com/repos/Theano/Theano/commits{/sha}","git_commits_url":"https://api.github.com/repos/Theano/Theano/git/commits{/sha}","comments_url":"https://api.github.com/repos/Theano/Theano/comments{/number}","issue_comment_url":"https://api.github.com/repos/Theano/Theano/issues/comments/{number}","contents_url":"https://api.github.com/repos/Theano/Theano/contents/{+path}","compare_url":"https://api.github.com/repos/Theano/Theano/compare/{base}...{head}","merges_url":"https://api.github.com/repos/Theano/Theano/merges","archive_url":"https://api.github.com/repos/Theano/Theano/{archive_format}{/ref}","downloads_url":"https://api.github.com/repos/Theano/Theano/downloads","issues_url":"https://api.github.com/repos/Theano/Theano/issues{/number}","pulls_url":"https://api.github.com/repos/Theano/Theano/pulls{/number}","milestones_url":"https://api.github.com/repos/Theano/Theano/milestones{/number}","notifications_url":"https://api.github.com/repos/Theano/Theano/notifications{?since,all,participating}","labels_url":"https://api.github.com/repos/Theano/Theano/labels{/name}","releases_url":"https://api.github.com/repos/Theano/Theano/releases{/id}","created_at":"2011-08-10T03:48:06Z","updated_at":"2014-08-18T22:09:40Z","pushed_at":"2014-08-18T18:23:38Z","git_url":"git://github.com/Theano/Theano.git","ssh_url":"git@github.com:Theano/Theano.git","clone_url":"https://github.com/Theano/Theano.git","svn_url":"https://github.com/Theano/Theano","homepage":"http://www.deeplearning.net/software/theano","size":37505,"stargazers_count":749,"watchers_count":749,"language":"Python","has_issues":true,"has_downloads":true,"has_wiki":true,"forks_count":273,"mirror_url":null,"open_issues_count":252,"forks":273,"open_issues":252,"watchers":749,"default_branch":"master"}},"_links":{"self":{"href":"https://api.github.com/repos/Theano/Theano/pulls/2033"},"html":{"href":"https://github.com/Theano/Theano/pull/2033"},"issue":{"href":"https://api.github.com/repos/Theano/Theano/issues/2033"},"comments":{"href":"https://api.github.com/repos/Theano/Theano/issues/2033/comments"},"review_comments":{"href":"https://api.github.com/repos/Theano/Theano/pulls/2033/comments"},"review_comment":{"href":"https://api.github.com/repos/Theano/Theano/pulls/comments/{number}"},"commits":{"href":"https://api.github.com/repos/Theano/Theano/pulls/2033/commits"},"statuses":{"href":"https://api.github.com/repos/Theano/Theano/statuses/c2db8b1dfbff90046fc54d16f34f80307a324625"}}}}