Implement ops and add functionality to bindings #70

dsmilkov · 2018-04-15T15:58:11Z

NOTE: Depends on tensorflow/tfjs-core#956

Implement: argMin, argMax, logicalXor, log1p, eluDer, clip, mod, round, sign, rsqrt, reciprocal, asinh, acosh, atanh, squaredDifference, expm1, atan2, conv2d, conv2dDerInput, conv2dDerFilter, maxPool, maxPoolBackprop, avgPool, avgPoolBackprop, tile, resizeBilinear, batchNorm, LRN, multinomial, softplus.
Implement depthwiseConv2D for dilations=1. dilations>1 requires spaceToBatch to be added to core
Implement the backend methods: time, memory.
Add binding functionality for types of attributes: float and list(int)
Test the ops using tfjs-core tests
Throw error if
- padding type is number since TF backend doesn't have kernels for that - only valid and same is supported. We support padding type number in the webgl and cpu backend.
- multinomial gets called with normalized probabilities. TF backend only supports logits (and you can't undo softmax), while we support both logits and probabilities in the webgl and cpu backend.

This change is

nkreeger · 2018-04-16T04:11:19Z

This is excellent - thank you so much! A few comments on the c++ ping me when they're updated and I'll rubber-stamp.

Review status: 0 of 4 files reviewed at latest revision, all discussions resolved.

binding/tfe_utils.cc, line 244 at r1 (raw file):

nullptr

Sorry these macros are funny - nullptr should probably be false or is_array. The 3rd param is the early return value. Honestly the only reason to return early is to bubble exceptions. Further up the call chain, calls into AssignOpAttr() have an exception-pending check. To make this easier, I'd do something like this:

inline bool IsArray(napi_env, napi_status& nstatus, napi_value* val) {
   // Use in-param nstatus
}

This way if an exception happens - it gets bubbled up through the nstatus passed in.

binding/tfe_utils.cc, line 300 at r1 (raw file):

if (IsArray(env, &js_value)) {

I wonder if we'll have to support this for TF_ATTR_FLOAT as well. Mind cleaning up this TODO: https://github.com/tensorflow/tfjs-node/blob/master/binding/tfe_utils.cc#L332

binding/tfe_utils.cc, line 302 at r1 (raw file):

auto

There is already a nstatus variable in scope on this method.

binding/tfe_utils.cc, line 304 at r1 (raw file):

auto data = new int64_t[length];

This actually allocates on the heap and you're on the hook to cleanup. I'd use std::unique_ptr like the python bindings do:

std::unique_ptr<int64_t[]> values(new int64_t[num_values]);

When values goes out-of-scope it automatically deletes the wrapped pointer.

binding/tfe_utils.cc, line 327 at r1 (raw file):

nstatus

Ditto here with the nstatus reference.

Comments from Reviewable

nsthorat · 2018-04-16T15:58:59Z

Review status: 0 of 4 files reviewed at latest revision, 5 unresolved discussions.

src/nodejs_kernel_backend.ts, line 325 at r1 (raw file):

  logicalXor(a: Tensor, b: Tensor): Tensor {
    // x ^ y = (x | y) & ~(x & y)
    return logicalOr(a, b).logicalAnd(logicalAnd(a, b).logicalNot());

I think we should do this at the op level and remove the kernel entirely, WDYT?

Same for other places you implement the kernel as a combination of other ops.

src/nodejs_kernel_backend.ts, line 660 at r1 (raw file):

      },
    ];
    return this.executeSingleOutput('MaxPoolGrad', opAttrs, [x, y, dy]) as

thoughts on renaming our kernel methods to align with TF?

So maxPoolBackprop => maxPoolGrad

src/run_tests.ts, line 32 at r1 (raw file):

const IGNORE_LIST: string[] = [
  'depthwiseConv2D',  // Requires space_to_batch() for dilation > 1.

You can refer to the issue

tensorflow/tfjs#161

Comments from Reviewable

dsmilkov · 2018-04-16T18:39:36Z

PTAL. All comments addressed

Review status: 0 of 4 files reviewed at latest revision, 8 unresolved discussions.

binding/tfe_utils.cc, line 244 at r1 (raw file):