GitHub PullRequestReviewCommentEvent at 2014-10-20 17:36:00
Repository: Theano
Actor: nouiz
Payload: {"action":"created","comment":{"url":"https://api.github.com/repos/Theano/Theano/pulls/comments/19099955","id":19099955,"diff_hunk":"@@ -1539,99 +1540,124 @@ def local_alloc_elemwise(node):\n     \"\"\"\n     if not isinstance(node.op, T.Elemwise):\n         return False\n+\n     if len(node.outputs) > 1:\n-        #This is a supposition this code make that I'm not sure is always true.\n+        # Ensure all outputs have the same broadcast pattern\n+        # This is a supposition that I'm not sure is always true.\n         assert all([list(o.type.broadcastable) == list(\n                     node.outputs[0].type.broadcastable) for o in\n                     node.outputs[1:]])\n \n+    # The broadcast pattern of the ouptut must match the broadcast pattern of\n+    # at least one of the inputs.\n     if not any([list(i.type.broadcastable) == list(\n-                node.outputs[0].type.broadcastable) for i in node.inputs]):\n+        node.outputs[0].type.broadcastable) for i in node.inputs]):\n         return False\n-    if not any([i.owner and (isinstance(i.owner.op, T.Alloc) or \\\n-                             (isinstance(i.owner.op, T.DimShuffle) and\n-                              i.owner.inputs[0].owner and \\\n-                              isinstance(i.owner.inputs[0].owner.op, T.Alloc)))\n+\n+    def dimshuffled_alloc(i):\n+        return (isinstance(i.owner.op, T.DimShuffle) and\n+                     i.owner.inputs[0].owner and \\\n+                         isinstance(i.owner.inputs[0].owner.op, T.Alloc))\n+\n+    # At least one input must have an owner that is either a T.Alloc or a\n+    # T.DimShuffle with an owner that is a T.Alloc -- otherwise there is\n+    # nothing to optimize.\n+    if not any([i.owner\n+                and (isinstance(i.owner.op, T.Alloc) or dimshuffled_alloc(i))\n                 for i in node.inputs]):\n         return False\n-    no_broad_idx = -1\n+\n+    ## Search for input that we can use as a baseline for the dimensions.\n+    assert_op_idx = -1\n     for idx, i in enumerate(node.inputs):\n-        if not i.owner:\n-            if list(i.type.broadcastable) == [False, ] * i.type.ndim:\n-                no_broad_idx = idx\n+        if i.type.broadcastable == node.outputs[0].type.broadcastable:\n+            # Prefer an input that is not a T.Alloc nor a T.DimShuffle of a\n+            # T.Alloc so that all allocs can be optimized.\n+            if not (i.owner\n+                    and (isinstance(i.owner.op, T.Alloc)\n+                         or dimshuffled_alloc(i))):\n+                assert_op_idx = idx\n                 break\n-            else:\n-                continue\n-        if not any(i.type.broadcastable) and not isinstance(i.owner.op,\n-                                                            T.Alloc):\n-            no_broad_idx = idx\n-            break\n-        elif list(i.type.broadcastable) == list(\n-            node.outputs[0].type.broadcastable) \\\n-            and not isinstance(i.owner.op, T.Alloc) \\\n-            and not (isinstance(i.owner.op, T.DimShuffle) and\n-                     i.owner.inputs[0].owner and \\\n-                         isinstance(i.owner.inputs[0].owner.op, T.Alloc)):\n-            no_broad_idx = idx\n-            break\n \n-    assert no_broad_idx >= 0\n-    assert_op = node.inputs[no_broad_idx]\n+    # It may be the case that only T.Allocs and T.DimShuffle of T.Allocs exist.\n+    if assert_op_idx < 0:\n+        # We want to optimize as many allocs as possible. When there is more\n+        # than one then do all but one.\n+        if len(node.inputs) > 1:\n+            assert_op_idx = 0  # The first one is as good as any to use.\n+\n+        else:\n+            # When there is only one input then we can optimize if the\n+            # broadcast patterns of the input and output match.\n+            i =  node.inputs[0]\n+            if i.type.broadcastable == node.outputs[0].type.broadcastable:\n+                new_i = []\n+                if isinstance(i.owner.op, T.Alloc):\n+                    new_i.append(i.owner.inputs[0])\n+                elif dimshuffled_alloc(i):\n+                    new_i.append(i.owner.inputs[0].owner.inputs[0])\n+\n+                assert(len(new_i) > 0)\n+                return node.op(*new_i,\n+                               return_list=True)\n+\n+            # Otherwise nothing can be done.\n+            return False\n+\n+    assert_op = node.inputs[assert_op_idx]\n     cmp_op = assert_op\n-    new = []\n+    new_i = []\n \n     for i in node.inputs:\n+        # Remove alloc\n         if (i.owner and isinstance(i.owner.op, T.Alloc)\n             and i.owner.inputs[0].type != i.owner.outputs[0].type):\n             # when i.owner.inputs[0].type == i.owner.outputs[0].type we\n             # will remove that alloc later\n \n             assert i.type.ndim == cmp_op.ndim\n-            if theano.config.experimental.local_alloc_elemwise_assert:\n+            if (theano.config.experimental.local_alloc_elemwise_assert\n+                and node.fgraph.shape_feature.same_shape(i, cmp_op)):\n                 assert_op = assert_(assert_op,\n                                     *[T.eq(i.shape[idx], cmp_op.shape[idx])\\\n                                           for idx in xrange(i.type.ndim) \\\n                                           if not i.type.broadcastable[idx]])\n-                new.append(i.owner.inputs[0])\n-        elif i.owner and isinstance(i.owner.op, T.DimShuffle) \\\n-                and i.owner.inputs[0].owner \\\n-                and isinstance(i.owner.inputs[0].owner.op, T.Alloc):\n+            new_i.append(i.owner.inputs[0])\n+\n+        # Remove Alloc in DimShuffle\n+        elif i.owner and dimshuffled_alloc(i):\n             assert i.type.ndim == cmp_op.type.ndim\n-            if theano.config.experimental.local_alloc_elemwise_assert:\n+            if (theano.config.experimental.local_alloc_elemwise_assert\n+                and node.fgraph.shape_feature.same_shape(i, cmp_op)):\n                 assert_op = assert_(assert_op,\n                                     *[T.eq(i.shape[idx], cmp_op.shape[idx])\n                                       for idx in xrange(i.type.ndim)\n                                       if not i.type.broadcastable[idx]])\n-            new.append(i.owner.inputs[0].owner.inputs[0])\n+            new_i.append(i.owner.inputs[0].owner.inputs[0])\n         else:\n-            new.append(i)\n-    new[no_broad_idx] = assert_op\n+            new_i.append(i)\n+    new_i[assert_op_idx] = assert_op\n     if theano.config.experimental.local_alloc_elemwise_assert:\n         assert assert_op.owner.op is assert_\n-    return [node.op(*new)]\n+    return node.op(*new_i, return_list=True)\n \n #TODO, global optimizer that lift the assert to the beginning of the graph.\n-#TODO, when all inputs can be optimized do all except one\n+#TODO, optimize all inputs when possible -- currently when all inputs have\n+# an alloc all but one is optimized.\n \n theano.configparser.AddConfigVar('experimental.local_alloc_elemwise',\n         \"If True enable the experimental optimization local_alloc_elemwise\",\n-        theano.configparser.BoolParam(False),\n-        in_c_key=False)\n-#This version if faster but not as save.\n+                                 theano.configparser.BoolParam(\n+                                     False,","path":"theano/tensor/opt.py","position":170,"original_position":170,"commit_id":"ebddd57b3d330d715eca769f213550e18dde3b0b","original_commit_id":"ebddd57b3d330d715eca769f213550e18dde3b0b","user":{"login":"nouiz","id":180987,"avatar_url":"https://avatars.githubusercontent.com/u/180987?v=2","gravatar_id":"","url":"https://api.github.com/users/nouiz","html_url":"https://github.com/nouiz","followers_url":"https://api.github.com/users/nouiz/followers","following_url":"https://api.github.com/users/nouiz/following{/other_user}","gists_url":"https://api.github.com/users/nouiz/gists{/gist_id}","starred_url":"https://api.github.com/users/nouiz/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/nouiz/subscriptions","organizations_url":"https://api.github.com/users/nouiz/orgs","repos_url":"https://api.github.com/users/nouiz/repos","events_url":"https://api.github.com/users/nouiz/events{/privacy}","received_events_url":"https://api.github.com/users/nouiz/received_events","type":"User","site_admin":false},"body":"It should be True by default. It is an error if it is false.","created_at":"2014-10-20T17:36:00Z","updated_at":"2014-10-20T17:36:00Z","html_url":"https://github.com/Theano/Theano/pull/2195#discussion_r19099955","pull_request_url":"https://api.github.com/repos/Theano/Theano/pulls/2195","_links":{"self":{"href":"https://api.github.com/repos/Theano/Theano/pulls/comments/19099955"},"html":{"href":"https://github.com/Theano/Theano/pull/2195#discussion_r19099955"},"pull_request":{"href":"https://api.github.com/repos/Theano/Theano/pulls/2195"}}},"pull_request":{"url":"https://api.github.com/repos/Theano/Theano/pulls/2195","id":23029562,"html_url":"https://github.com/Theano/Theano/pull/2195","diff_url":"https://github.com/Theano/Theano/pull/2195.diff","patch_url":"https://github.com/Theano/Theano/pull/2195.patch","issue_url":"https://api.github.com/repos/Theano/Theano/issues/2195","number":2195,"state":"open","locked":false,"title":"Updated local_alloc_elemwise to remove all allocs when possible and to a...","user":{"login":"daemonmaker","id":2964386,"avatar_url":"https://avatars.githubusercontent.com/u/2964386?v=2","gravatar_id":"","url":"https://api.github.com/users/daemonmaker","html_url":"https://github.com/daemonmaker","followers_url":"https://api.github.com/users/daemonmaker/followers","following_url":"https://api.github.com/users/daemonmaker/following{/other_user}","gists_url":"https://api.github.com/users/daemonmaker/gists{/gist_id}","starred_url":"https://api.github.com/users/daemonmaker/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/daemonmaker/subscriptions","organizations_url":"https://api.github.com/users/daemonmaker/orgs","repos_url":"https://api.github.com/users/daemonmaker/repos","events_url":"https://api.github.com/users/daemonmaker/events{/privacy}","received_events_url":"https://api.github.com/users/daemonmaker/received_events","type":"User","site_admin":false},"body":"...dd assert only when we cannot determine whether the shapes will be correct.\r\n\r\nDeprecated experimental.local_alloc_elemwise and enabled local_alloc_elemwise by default.\r\n\r\nThis is just the start. Next to come are the associated tests.","created_at":"2014-10-20T17:07:58Z","updated_at":"2014-10-20T17:36:00Z","closed_at":null,"merged_at":null,"merge_commit_sha":"5de68444f5fd2f32f04b9500e1e02567d95cf2cb","assignee":null,"milestone":null,"commits_url":"https://api.github.com/repos/Theano/Theano/pulls/2195/commits","review_comments_url":"https://api.github.com/repos/Theano/Theano/pulls/2195/comments","review_comment_url":"https://api.github.com/repos/Theano/Theano/pulls/comments/{number}","comments_url":"https://api.github.com/repos/Theano/Theano/issues/2195/comments","statuses_url":"https://api.github.com/repos/Theano/Theano/statuses/ebddd57b3d330d715eca769f213550e18dde3b0b","head":{"label":"daemonmaker:issue1903","ref":"issue1903","sha":"ebddd57b3d330d715eca769f213550e18dde3b0b","user":{"login":"daemonmaker","id":2964386,"avatar_url":"https://avatars.githubusercontent.com/u/2964386?v=2","gravatar_id":"","url":"https://api.github.com/users/daemonmaker","html_url":"https://github.com/daemonmaker","followers_url":"https://api.github.com/users/daemonmaker/followers","following_url":"https://api.github.com/users/daemonmaker/following{/other_user}","gists_url":"https://api.github.com/users/daemonmaker/gists{/gist_id}","starred_url":"https://api.github.com/users/daemonmaker/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/daemonmaker/subscriptions","organizations_url":"https://api.github.com/users/daemonmaker/orgs","repos_url":"https://api.github.com/users/daemonmaker/repos","events_url":"https://api.github.com/users/daemonmaker/events{/privacy}","received_events_url":"https://api.github.com/users/daemonmaker/received_events","type":"User","site_admin":false},"repo":{"id":17116690,"name":"Theano","full_name":"daemonmaker/Theano","owner":{"login":"daemonmaker","id":2964386,"avatar_url":"https://avatars.githubusercontent.com/u/2964386?v=2","gravatar_id":"","url":"https://api.github.com/users/daemonmaker","html_url":"https://github.com/daemonmaker","followers_url":"https://api.github.com/users/daemonmaker/followers","following_url":"https://api.github.com/users/daemonmaker/following{/other_user}","gists_url":"https://api.github.com/users/daemonmaker/gists{/gist_id}","starred_url":"https://api.github.com/users/daemonmaker/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/daemonmaker/subscriptions","organizations_url":"https://api.github.com/users/daemonmaker/orgs","repos_url":"https://api.github.com/users/daemonmaker/repos","events_url":"https://api.github.com/users/daemonmaker/events{/privacy}","received_events_url":"https://api.github.com/users/daemonmaker/received_events","type":"User","site_admin":false},"private":false,"html_url":"https://github.com/daemonmaker/Theano","description":"Optimizing GPU-meta-programming code generating array oriented optimizing math compiler in Python","fork":true,"url":"https://api.github.com/repos/daemonmaker/Theano","forks_url":"https://api.github.com/repos/daemonmaker/Theano/forks","keys_url":"https://api.github.com/repos/daemonmaker/Theano/keys{/key_id}","collaborators_url":"https://api.github.com/repos/daemonmaker/Theano/collaborators{/collaborator}","teams_url":"https://api.github.com/repos/daemonmaker/Theano/teams","hooks_url":"https://api.github.com/repos/daemonmaker/Theano/hooks","issue_events_url":"https://api.github.com/repos/daemonmaker/Theano/issues/events{/number}","events_url":"https://api.github.com/repos/daemonmaker/Theano/events","assignees_url":"https://api.github.com/repos/daemonmaker/Theano/assignees{/user}","branches_url":"https://api.github.com/repos/daemonmaker/Theano/branches{/branch}","tags_url":"https://api.github.com/repos/daemonmaker/Theano/tags","blobs_url":"https://api.github.com/repos/daemonmaker/Theano/git/blobs{/sha}","git_tags_url":"https://api.github.com/repos/daemonmaker/Theano/git/tags{/sha}","git_refs_url":"https://api.github.com/repos/daemonmaker/Theano/git/refs{/sha}","trees_url":"https://api.github.com/repos/daemonmaker/Theano/git/trees{/sha}","statuses_url":"https://api.github.com/repos/daemonmaker/Theano/statuses/{sha}","languages_url":"https://api.github.com/repos/daemonmaker/Theano/languages","stargazers_url":"https://api.github.com/repos/daemonmaker/Theano/stargazers","contributors_url":"https://api.github.com/repos/daemonmaker/Theano/contributors","subscribers_url":"https://api.github.com/repos/daemonmaker/Theano/subscribers","subscription_url":"https://api.github.com/repos/daemonmaker/Theano/subscription","commits_url":"https://api.github.com/repos/daemonmaker/Theano/commits{/sha}","git_commits_url":"https://api.github.com/repos/daemonmaker/Theano/git/commits{/sha}","comments_url":"https://api.github.com/repos/daemonmaker/Theano/comments{/number}","issue_comment_url":"https://api.github.com/repos/daemonmaker/Theano/issues/comments/{number}","contents_url":"https://api.github.com/repos/daemonmaker/Theano/contents/{+path}","compare_url":"https://api.github.com/repos/daemonmaker/Theano/compare/{base}...{head}","merges_url":"https://api.github.com/repos/daemonmaker/Theano/merges","archive_url":"https://api.github.com/repos/daemonmaker/Theano/{archive_format}{/ref}","downloads_url":"https://api.github.com/repos/daemonmaker/Theano/downloads","issues_url":"https://api.github.com/repos/daemonmaker/Theano/issues{/number}","pulls_url":"https://api.github.com/repos/daemonmaker/Theano/pulls{/number}","milestones_url":"https://api.github.com/repos/daemonmaker/Theano/milestones{/number}","notifications_url":"https://api.github.com/repos/daemonmaker/Theano/notifications{?since,all,participating}","labels_url":"https://api.github.com/repos/daemonmaker/Theano/labels{/name}","releases_url":"https://api.github.com/repos/daemonmaker/Theano/releases{/id}","created_at":"2014-02-23T20:47:11Z","updated_at":"2014-08-20T20:13:16Z","pushed_at":"2014-10-20T16:48:03Z","git_url":"git://github.com/daemonmaker/Theano.git","ssh_url":"git@github.com:daemonmaker/Theano.git","clone_url":"https://github.com/daemonmaker/Theano.git","svn_url":"https://github.com/daemonmaker/Theano","homepage":"http://www.deeplearning.net/software/theano","size":22567,"stargazers_count":0,"watchers_count":0,"language":"Python","has_issues":false,"has_downloads":true,"has_wiki":true,"has_pages":false,"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":"85741b9e7925827e56bfd054d732ed2ccb5116c2","user":{"login":"Theano","id":970517,"avatar_url":"https://avatars.githubusercontent.com/u/970517?v=2","gravatar_id":"","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":"","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-10-20T13:16:23Z","pushed_at":"2014-10-20T17:02:57Z","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":41881,"stargazers_count":846,"watchers_count":846,"language":"Python","has_issues":true,"has_downloads":true,"has_wiki":true,"has_pages":false,"forks_count":320,"mirror_url":null,"open_issues_count":281,"forks":320,"open_issues":281,"watchers":846,"default_branch":"master"}},"_links":{"self":{"href":"https://api.github.com/repos/Theano/Theano/pulls/2195"},"html":{"href":"https://github.com/Theano/Theano/pull/2195"},"issue":{"href":"https://api.github.com/repos/Theano/Theano/issues/2195"},"comments":{"href":"https://api.github.com/repos/Theano/Theano/issues/2195/comments"},"review_comments":{"href":"https://api.github.com/repos/Theano/Theano/pulls/2195/comments"},"review_comment":{"href":"https://api.github.com/repos/Theano/Theano/pulls/comments/{number}"},"commits":{"href":"https://api.github.com/repos/Theano/Theano/pulls/2195/commits"},"statuses":{"href":"https://api.github.com/repos/Theano/Theano/statuses/ebddd57b3d330d715eca769f213550e18dde3b0b"}}}}