Skip to content
This repository was archived by the owner on Aug 15, 2019. It is now read-only.

Conversation

@dsmilkov
Copy link
Contributor

@dsmilkov dsmilkov commented Aug 13, 2017

This change is Reviewable

@dsmilkov dsmilkov requested a review from nsthorat August 13, 2017 21:12
@nsthorat
Copy link
Contributor

:lgtm_strong:


Reviewed 17 of 17 files at r1.
Review status: all files reviewed at latest revision, 2 unresolved discussions.


src/math/math.ts, line 717 at r1 (raw file):

        `Error in scalarDividedByArray: first argument must be rank 0, but ` +
            `got NDArray of rank ${c.rank}.`);
    return this.divide(c, a) as T;

Can you update the graph methods, add, multiply, divide, sub to use the broadcast methods? You'll clean up a lot of code there


src/math/webgl/shader_compiler.ts, line 312 at r1 (raw file):

      vec2 resTexRC = floor(gl_FragCoord.yx);
      float index = dot(resTexRC, vec2(${outTexShape[1]}.0, 1.0));
      index = mod(index, ${inSize}.0);

Tiny optimization: pass the broadcast bit in and splice this


Comments from Reviewable

@dsmilkov
Copy link
Contributor Author

Reviewed 10 of 17 files at r1.
Review status: 13 of 17 files reviewed at latest revision, 1 unresolved discussion, all commit checks successful.


src/math/math.ts, line 717 at r1 (raw file):

Previously, nsthorat (Nikhil Thorat) wrote…

Can you update the graph methods, add, multiply, divide, sub to use the broadcast methods? You'll clean up a lot of code there

Per offline discussion, backprop is non-trivial for general broadcasting - better to do in another PR.


src/math/webgl/shader_compiler.ts, line 312 at r1 (raw file):

Previously, nsthorat (Nikhil Thorat) wrote…

Tiny optimization: pass the broadcast bit in and splice this

Done.


Comments from Reviewable

@dsmilkov dsmilkov merged commit c254d95 into master Aug 13, 2017
@dsmilkov
Copy link
Contributor Author

This migrates more ops, tracked in #18

@dsmilkov dsmilkov deleted the logical branch August 13, 2017 23:48
mnottheone pushed a commit to mnottheone/deeplearnjs that referenced this pull request Dec 1, 2018
mnottheone pushed a commit to mnottheone/deeplearnjs that referenced this pull request Dec 1, 2018
tensorflow#40)

* migrate binary op to logical and add broadcasting

* fix lint and build error

* optimize the getAAtOutputCoord sampler and getFlat sampler
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants