{"action":"created","issue":{"url":"https://api.github.com/repos/tensorflow/tfjs-core/issues/887","repository_url":"https://api.github.com/repos/tensorflow/tfjs-core","labels_url":"https://api.github.com/repos/tensorflow/tfjs-core/issues/887/labels{/name}","comments_url":"https://api.github.com/repos/tensorflow/tfjs-core/issues/887/comments","events_url":"https://api.github.com/repos/tensorflow/tfjs-core/issues/887/events","html_url":"https://github.com/tensorflow/tfjs-core/pull/887","id":308106509,"number":887,"title":"Binary Ops: Adds dl.mod operation","user":{"login":"ManrajGrover","id":5673050,"avatar_url":"https://avatars3.githubusercontent.com/u/5673050?v=4","gravatar_id":"","url":"https://api.github.com/users/ManrajGrover","html_url":"https://github.com/ManrajGrover","followers_url":"https://api.github.com/users/ManrajGrover/followers","following_url":"https://api.github.com/users/ManrajGrover/following{/other_user}","gists_url":"https://api.github.com/users/ManrajGrover/gists{/gist_id}","starred_url":"https://api.github.com/users/ManrajGrover/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/ManrajGrover/subscriptions","organizations_url":"https://api.github.com/users/ManrajGrover/orgs","repos_url":"https://api.github.com/users/ManrajGrover/repos","events_url":"https://api.github.com/users/ManrajGrover/events{/privacy}","received_events_url":"https://api.github.com/users/ManrajGrover/received_events","type":"User","site_admin":false},"labels":[],"state":"open","locked":false,"assignee":null,"assignees":[],"milestone":null,"comments":1,"created_at":"2018-03-23T16:54:51Z","updated_at":"2018-04-02T00:19:22Z","closed_at":null,"author_association":"CONTRIBUTOR","pull_request":{"url":"https://api.github.com/repos/tensorflow/tfjs-core/pulls/887","html_url":"https://github.com/tensorflow/tfjs-core/pull/887","diff_url":"https://github.com/tensorflow/tfjs-core/pull/887.diff","patch_url":"https://github.com/tensorflow/tfjs-core/pull/887.patch"},"body":"This PR adds `dl.mod` operation to the API.\r\n\r\nFixes #884\n\n<!-- Reviewable:start -->\n---\nThis change is [<img src=\"https://reviewable.io/review_button.svg\" height=\"34\" align=\"absmiddle\" alt=\"Reviewable\"/>](https://reviewable.io/reviews/pair-code/deeplearnjs/887)\n<!-- Reviewable:end -->\n"},"comment":{"url":"https://api.github.com/repos/tensorflow/tfjs-core/issues/comments/377828035","html_url":"https://github.com/tensorflow/tfjs-core/pull/887#issuecomment-377828035","issue_url":"https://api.github.com/repos/tensorflow/tfjs-core/issues/887","id":377828035,"user":{"login":"dsmilkov","id":2294279,"avatar_url":"https://avatars2.githubusercontent.com/u/2294279?v=4","gravatar_id":"","url":"https://api.github.com/users/dsmilkov","html_url":"https://github.com/dsmilkov","followers_url":"https://api.github.com/users/dsmilkov/followers","following_url":"https://api.github.com/users/dsmilkov/following{/other_user}","gists_url":"https://api.github.com/users/dsmilkov/gists{/gist_id}","starred_url":"https://api.github.com/users/dsmilkov/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/dsmilkov/subscriptions","organizations_url":"https://api.github.com/users/dsmilkov/orgs","repos_url":"https://api.github.com/users/dsmilkov/repos","events_url":"https://api.github.com/users/dsmilkov/events{/privacy}","received_events_url":"https://api.github.com/users/dsmilkov/received_events","type":"User","site_admin":false},"created_at":"2018-04-02T00:19:22Z","updated_at":"2018-04-02T00:19:22Z","author_association":"MEMBER","body":"Nice work Manraj. Few comments, but we are very close.\n\n---\n\nReviewed 7 of 8 files at r2.\nReview status: 7 of 8 files reviewed at latest revision, all discussions resolved, some commit checks failed.\n\n---\n\n*[src/kernels/webgl/binaryop_gpu.ts, line 79 at r2](https://beta.reviewable.io/reviews/tensorflow/tfjs-core/887#-L92frOhASX8R4GisIyO:-L92frOhASX8R4GisIyP:buu1x6) ([raw file](https://github.com/tensorflow/tfjs-core/blob/8a9cc02eec7df13a64348f0c4bf4c5a3584415fe/src/kernels/webgl/binaryop_gpu.ts#L79)):*\n> ```XML\n>     return rem;\n>   } else {\n>     return mod(rem + b, b);\n> ```\n\ndo you need this check in glsl and the special case? Based on https://www.khronos.org/registry/OpenGL-Refpages/gl4/html/mod.xhtml it looks like the built-in `mod(x,y)` is computed as `x - y * floor(x/y)` which is exactly aligned with TF's mod.\n\n---\n\n*[src/ops/binary_ops.ts, line 373 at r2](https://beta.reviewable.io/reviews/tensorflow/tfjs-core/887#-L92i-LmAG0xjgyPdFAa:-L92i-LmAG0xjgyPdFAb:biyd2yq) ([raw file](https://github.com/tensorflow/tfjs-core/blob/8a9cc02eec7df13a64348f0c4bf4c5a3584415fe/src/ops/binary_ops.ts#L373)):*\n> ```XML\n>    * const b = dl.tensor1d([1, 2, 9, 4]);\n>    *\n>    * a.mod(b).print();  // or dl.mod(a, b)\n> ```\n\n`tf.` in the doc snippet.\n\n---\n\n*[src/ops/binary_ops.ts, line 393 at r2](https://beta.reviewable.io/reviews/tensorflow/tfjs-core/887#-L92h4MKB3mw4ylOtgCr:-L92h4MKB3mw4ylOtgCs:b7ox9t8) ([raw file](https://github.com/tensorflow/tfjs-core/blob/8a9cc02eec7df13a64348f0c4bf4c5a3584415fe/src/ops/binary_ops.ts#L393)):*\n> ```XML\n>     broadcast_util.assertAndGetBroadcastShape(a.shape, b.shape);\n>     const der = (dy: Tensor) => {\n>       const derA = () => dy.mul(onesLike(a));\n> ```\n\nmod supports broadcasting, which means you'll have to reduce the derA to have the shape of `a`, which means you have to dy.sum(reductionAxis). See gradient in our tf.mul() for an example: https://github.com/tensorflow/tfjs-core/blob/master/src/ops/binary_ops.ts#L259\n\n---\n\n*[src/ops/binary_ops_test.ts, line 341 at r2](https://beta.reviewable.io/reviews/tensorflow/tfjs-core/887#-L92e3Z31GnvkFiFxv5B:-L92e3Z31GnvkFiFxv5C:bxg7xof) ([raw file](https://github.com/tensorflow/tfjs-core/blob/8a9cc02eec7df13a64348f0c4bf4c5a3584415fe/src/ops/binary_ops_test.ts#L341)):*\n> ```XML\n> \n>     expect(result.shape).toEqual(a.shape);\n>     expectArraysClose(result, [0.09999999, 0.19999996, 0.15, 0.05000016]);\n> ```\n\ninstead of using values like 0.09999 and 0.199999996, use the correct expected values like 0.1 and 0.2. If the test fails with those values, it will be highly informative, since it tells us that we should improve the implementation to be more numerically stable.\n\n---\n\n*[src/ops/binary_ops_test.ts, line 376 at r2](https://beta.reviewable.io/reviews/tensorflow/tfjs-core/887#-L92f0kO2Y0rTlFnKi68:-L92f0kO2Y0rTlFnKi69:b-g4epwr) ([raw file](https://github.com/tensorflow/tfjs-core/blob/8a9cc02eec7df13a64348f0c4bf4c5a3584415fe/src/ops/binary_ops_test.ts#L376)):*\n> ```XML\n> \n>     expect(result.shape).toEqual(a.shape);\n>     expectArraysClose(result, [0.5, 0.0999999, 0.5, 0.20000017]);\n> ```\n\nsame here: use the correct math values e.g. 0.1. Likewise for the other tests in this file.\n\n---\n\n*[src/ops/binary_ops_test.ts, line 461 at r2](https://beta.reviewable.io/reviews/tensorflow/tfjs-core/887#-L92htrMAQIJCIv4fWme:-L92htrMAQIJCIv4fWmf:b-wnkxtb) ([raw file](https://github.com/tensorflow/tfjs-core/blob/8a9cc02eec7df13a64348f0c4bf4c5a3584415fe/src/ops/binary_ops_test.ts#L461)):*\n> ```XML\n>       3 * -1 * Math.floor(0.7 / 0.7), 4 * -1 * Math.floor(0.91 / 0.15)\n>     ]);\n>   });\n> ```\n\nadd a gradient test that uses broadcasting.\n\n---\n\n\n*Comments from [Reviewable](https://beta.reviewable.io/reviews/tensorflow/tfjs-core/887#-:-L92i38D22vbXtDzI-cJ:b-ncc2dn)*\n<!-- Sent from Reviewable.io -->\n"}}
{"actor":{"display_login":"dsmilkov"}}