Modularize stack, unstack, reshape, expandDims, squeeze

[annxingyuan]

• Not removing reshape from Tensor interface to avoid circular dependency.

To see the logs from the Cloud Build CI, please join either our discussion or announcement mailing list.

---

This change is 

[lina128]

Reviewed 65 of 72 files at r1.
Reviewable status: complete! 1 of 1 approvals obtained (waiting on @annxingyuan, @lina128, and @tafsiri)

---

tfjs-core/src/gradients/Reshape_grad.ts, line 24 at r1 (raw file):

export const reshapeGradConfig: GradConfig = {
  kernelName: Reshape,
  inputsToSave: ['tensor'],

Should this be ['x']?

---

tfjs-core/src/public/chained_ops/stack.ts, line 27 at r1 (raw file):

}

Tensor.prototype.stack = function<T extends Tensor>(

Just a comment, it's interesting that the chained api doesn't allow stacking a list of tensors. Will the chained ops have doccomment anywhere?

[tafsiri]

Reviewed 70 of 72 files at r1.
Reviewable status: complete! 2 of 1 approvals obtained (waiting on @annxingyuan, @lina128, and @tafsiri)

---

tfjs-core/src/ops/expand_dims.ts, line 41 at r1 (raw file):

 */
/** @doc {heading: 'Tensors', subheading: 'Transformations'} */
function expandDims_<R2 extends Rank>(

Why R2 here (as opposed to R)?

---

tfjs-core/src/ops/squeeze.ts, line 42 at r1 (raw file):

function squeeze_<T extends Tensor>(x: Tensor|TensorLike, axis?: number[]): T {
  const $x = convertToTensor(x, 'x', 'squeeze');
  return reshape($x, util.squeezeShape($x.shape, axis).newShape) as T;

nit, could you just import squeezeShape from util?

---

tfjs-core/src/ops/stack.ts, line 65 at r1 (raw file):

  $tensors.forEach(t => {
    util.assert(

nit/suggestion, add this assertion to the loop above.

---

tfjs-core/src/ops/unstack.ts, line 44 at r1 (raw file):

/** @doc {heading: 'Tensors', subheading: 'Slicing and Joining'} */
function unstack_(x: Tensor|TensorLike, axis = 0): Tensor[] {
  axis = axis || 0;

nit: this seems unnecessary given the default argument above

---

tfjs-core/src/public/chained_ops/stack.ts, line 27 at r1 (raw file):

Previously, lina128 (Na Li) wrote…

Just a comment, it's interesting that the chained api doesn't allow stacking a list of tensors. Will the chained ops have doccomment anywhere?

Yeah it looks like it should also take an array of tensors for x. Even if it didn't before, we can fix that now.

[annxingyuan]

Reviewable status: complete! 2 of 1 approvals obtained (waiting on @lina128 and @tafsiri)

---

tfjs-core/src/gradients/Reshape_grad.ts, line 24 at r1 (raw file):

Previously, lina128 (Na Li) wrote…

Should this be ['x']?

According to https://raw.githubusercontent.com/tensorflow/tensorflow/master/tensorflow/core/ops/ops.pbtxt - the input to Reshape is called tensor.

---

tfjs-core/src/ops/expand_dims.ts, line 41 at r1 (raw file):

Previously, tafsiri (Yannick Assogba) wrote…

Why R2 here (as opposed to R)?

I had copied that over from array_ops.ts - but you're right - I can't think of a reason why it must be R2. Changed.

---

tfjs-core/src/ops/squeeze.ts, line 42 at r1 (raw file):

Previously, tafsiri (Yannick Assogba) wrote…

nit, could you just import squeezeShape from util?

Done

---

tfjs-core/src/ops/stack.ts, line 65 at r1 (raw file):

Previously, tafsiri (Yannick Assogba) wrote…

nit/suggestion, add this assertion to the loop above.

Done

---

tfjs-core/src/ops/unstack.ts, line 44 at r1 (raw file):

Previously, tafsiri (Yannick Assogba) wrote…

nit: this seems unnecessary given the default argument above

Done

---

tfjs-core/src/public/chained_ops/stack.ts, line 27 at r1 (raw file):

Previously, tafsiri (Yannick Assogba) wrote…

Yeah it looks like it should also take an array of tensors for x. Even if it didn't before, we can fix that now.

Done

[lina128]

Reviewable status: complete! 2 of 1 approvals obtained (waiting on @annxingyuan, @lina128, and @tafsiri)

---

tfjs-core/src/ops/reshape.ts, line 63 at r2 (raw file):

      () => 'new shape and old shape must have the same number of elements.');

  const inputs: ReshapeInputs = {tensor: $x};

Are we following the arg names in ops.pbtxt too? Because most of their input tensors are named input, and most of our input tensors are named x. Does this mean we need to change all the input arg to align with ops.pbtxt? https://raw.githubusercontent.com/tensorflow/tensorflow/master/tensorflow/core/ops/ops.pbtxt

[annxingyuan]

@lina128 Hmm, I guess I thought we were strictly following that, I think sometimes their input tensors are named x as well. I don't think we need to go back and change anything we've already modularized, but maybe we should use the name tensor going forward if it appears in ops.pbtxt? Or maybe we were avoiding using tensor due to name conflicts? Do you have thoughts on this @tafsiri ?

[tafsiri]

lets chat on gvc. personally i don't think it matters too much but we should pick a convention. I our assumptions about how well things could align at the beginning might have changed. i'll ping you on chat.