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

Expose GPGPUProgram but do it right this time #1203

Merged
merged 11 commits into from
Aug 2, 2018

Conversation

aman-tiwari
Copy link
Contributor

@aman-tiwari aman-tiwari commented Aug 2, 2018

Description

See #1202


For repository owners only:

Please remember to apply all applicable tags to your pull request.
Tags: FEATURE, BREAKING, BUG, PERF, DEV, DOC, SECURITY

For more info see: https://github.com/tensorflow/tfjs/blob/master/DEVELOPMENT.md


This change is Reviewable

Copy link
Contributor

@dsmilkov dsmilkov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 0 of 1 approvals obtained (waiting on @aman-tiwari)


src/kernels/webgl/webgl_custom_op_test.ts, line 64 at r2 (raw file):

    const backward = (dy: T) => {
      return {
        x: () => tf.tidy(() => {

You don't need tidy since we should wrap that for you in the engine when we backpropagate.


src/kernels/webgl/webgl_custom_op_test.ts, line 71 at r2 (raw file):

    };

    const res = tf.ENV.engine.runKernel(forward, {x}, backward);

Instead of runKernel, how about using tf.customGrad() which is already documented and aligned with https://www.tensorflow.org/api_docs/python/tf/custom_gradient

Copy link
Contributor Author

@aman-tiwari aman-tiwari left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 0 of 1 approvals obtained (waiting on @aman-tiwari)


src/kernels/webgl/webgl_custom_op_test.ts, line 71 at r2 (raw file):

Previously, dsmilkov (Daniel Smilkov) wrote…

Instead of runKernel, how about using tf.customGrad() which is already documented and aligned with https://www.tensorflow.org/api_docs/python/tf/custom_gradient

customGrad only works if the things inside it call runKernel at some point (according to @nsthorat)

Copy link
Contributor

@dsmilkov dsmilkov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 0 of 1 approvals obtained (waiting on @aman-tiwari and @dsmilkov)


src/kernels/webgl/webgl_custom_op_test.ts, line 64 at r2 (raw file):

Previously, dsmilkov (Daniel Smilkov) wrote…

You don't need tidy since we should wrap that for you in the engine when we backpropagate.

Move the construction of the backpropProgram outside of the derX function so we don't recreate it every time we backprop.


src/kernels/webgl/webgl_custom_op_test.ts, line 71 at r2 (raw file):

Previously, aman-tiwari (Aman Tiwari) wrote…

customGrad only works if the things inside it call runKernel at some point (according to @nsthorat)

it should work without runKernel, unless there is a bug. Anyhow, runKernel() is good for now, but we should revisit customGrad() and figure out what's going on.

Copy link
Contributor Author

@aman-tiwari aman-tiwari left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 0 of 1 approvals obtained (waiting on @aman-tiwari)


src/kernels/webgl/webgl_custom_op_test.ts, line 71 at r2 (raw file):

Previously, dsmilkov (Daniel Smilkov) wrote…

it should work without runKernel, unless there is a bug. Anyhow, runKernel() is good for now, but we should revisit customGrad() and figure out what's going on.

@nsthorat is looking at the bug now

Copy link
Contributor

@dsmilkov dsmilkov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 1 of 1 files at r3.
Reviewable status: 0 of 1 approvals obtained (waiting on @dsmilkov)

@dsmilkov dsmilkov merged commit f2ebe9c into tensorflow:master Aug 2, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
2 participants