add support for tensor list #3411

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Merged

pyu10055 merged 5 commits into master from tensor_list

Jun 9, 2020

Collaborator

pyu10055 commented Jun 8, 2020 •

edited by nsthorat

Loading

This is the base class for TF TensorList ops.

https://www.tensorflow.org/api_docs/python/tf/raw_ops/TensorListSetItem

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

This change is


          add support for tensor list

03ba235

pyu10055 requested a review from lina128

June 8, 2020 23:26

googlebot added the cla: yes label

pyu10055 added 3 commits

June 8, 2020 16:26


          Merge branch 'master' into tensor_list


          updated js docs

4c48d66


          fix lint error

d9ed20e

lina128 reviewed

View reviewed changes

Collaborator

lina128 left a comment

Reviewed 1 of 4 files at r1.
Reviewable status: 0 of 1 approvals obtained (waiting on @lina128 and @pyu10055)

tfjs-converter/src/executor/tensor_list.ts, line 96 at r1 (raw file):

  }

  popBack(elementShape: number[], elementDtype: DataType): Tensor {

Doccomment

tfjs-converter/src/executor/tensor_list.ts, line 112 at r1 (raw file):

  }

  pushBack(tensor: Tensor) {

Doccomment.

tfjs-converter/src/executor/tensor_list.ts, line 127 at r1 (raw file):

  }

  resize(size: number) {

Doccomment

tfjs-converter/src/executor/tensor_list.ts, line 18 at r2 (raw file):

 */

import {DataType, stack, Tensor, tensor} from '@tensorflow/tfjs-core';

Combine all the imports from tfjs-core in one line?

tfjs-converter/src/executor/tensor_list.ts, line 54 at r2 (raw file):

      public elementDtype: DataType, public maxNumElements = -1) {}

  private _typeName: string;

I think we are moving away from using underscore for private variables.

tfjs-converter/src/executor/tensor_list.ts, line 57 at r2 (raw file):

  get typeName() {
    return this._typeName;

Remove underscore.

tfjs-converter/src/executor/tensor_list.ts, line 60 at r2 (raw file):

  }

  // Get a new TensorList containing a copy of the underlying tensor container.

Change from annotation to doccomment. This is a public method.

tfjs-converter/src/executor/tensor_list.ts, line 66 at r2 (raw file):

  }

  size() {

Doccomment.

tfjs-converter/src/executor/tensor_list.ts, line 71 at r2 (raw file):

  /**
   * return a tensor that stacks all element together.

Return a tensor that stacks a list of rank-R tf.Tensors into one rank-(R+1) tf.Tensor.

tfjs-converter/src/executor/tensor_list.ts, line 78 at r2 (raw file):

  stack(elementShape: number[], elementDtype: DataType, numElements = -1):
      Tensor {
    if (elementDtype !== this.elementDtype) {

If unequal always result in an error, then why do we pass in the value? Can we just use this.elementDtype?

tfjs-converter/src/executor/tensor_list.ts, line 192 at r2 (raw file):

  /**
   * Return selected values in the TensorList as a packed Tensor. All of

Return selected tensors in the TensorList as a stacked tensor. All of selected tensors ...

tfjs-converter/src/executor/tensor_list.ts, line 208 at r2 (raw file):

        this.elementShape, elementShape, 'TensorList shape mismatch: ');

    indices = indices.slice(0, this.size());

Can we add an annotation why we need to do this? It is not obvious from the code.

tfjs-converter/src/executor/tensor_list.ts, line 235 at r2 (raw file):

    if (this.size() === 0) {
      return tensor([], [0].concat(this.elementShape));

Why the shape is not just [0]?

tfjs-converter/src/executor/tensor_list.ts, line 270 at r2 (raw file):

/**
 * return a TensorList of the given size with empty elements.

Capitalize.

tfjs-converter/src/executor/tensor_list.ts, line 282 at r2 (raw file):

/**
 * Scatter the values of a Tensor in specific indices of a TensorList.

Put tensors at specific indices of a stacked tensor into a TensorList.

tfjs-converter/src/executor/tensor_list.ts, line 313 at r2 (raw file):

/**
 * Split the values of a Tensor into the TensorArray.

into a

tfjs-converter/src/executor/tensor_list.ts, line 314 at r2 (raw file):

/**
 * Split the values of a Tensor into the TensorArray.
 * @param length number[] with the lengths to use when splitting value along

remove with?

tfjs-converter/src/executor/tensor_list.ts, line 316 at r2 (raw file):

 * @param length number[] with the lengths to use when splitting value along
 *    its first dimension.
 * @param tensor Tensor, the tensor to split.

Remove Tensor, ?


          addressed comments

94a26ab

pyu10055 commented

View reviewed changes

Collaborator Author

pyu10055 left a comment

Reviewable status: 0 of 1 approvals obtained (waiting on @lina128)

tfjs-converter/src/executor/tensor_list.ts, line 96 at r1 (raw file):

Previously, lina128 (Na Li) wrote…

Doccomment

Done.

tfjs-converter/src/executor/tensor_list.ts, line 112 at r1 (raw file):

Previously, lina128 (Na Li) wrote…

Doccomment.

Done.

tfjs-converter/src/executor/tensor_list.ts, line 127 at r1 (raw file):

Previously, lina128 (Na Li) wrote…

Doccomment

Done.

tfjs-converter/src/executor/tensor_list.ts, line 18 at r2 (raw file):

Previously, lina128 (Na Li) wrote…

Combine all the imports from tfjs-core in one line?

Done.

tfjs-converter/src/executor/tensor_list.ts, line 54 at r2 (raw file):

Previously, lina128 (Na Li) wrote…

I think we are moving away from using underscore for private variables.

Done.

tfjs-converter/src/executor/tensor_list.ts, line 57 at r2 (raw file):

Previously, lina128 (Na Li) wrote…

Remove underscore.

Done.

tfjs-converter/src/executor/tensor_list.ts, line 60 at r2 (raw file):

Previously, lina128 (Na Li) wrote…

Change from annotation to doccomment. This is a public method.

Done.

tfjs-converter/src/executor/tensor_list.ts, line 66 at r2 (raw file):

Previously, lina128 (Na Li) wrote…

Doccomment.

Done.

tfjs-converter/src/executor/tensor_list.ts, line 71 at r2 (raw file):

Previously, lina128 (Na Li) wrote…

Return a tensor that stacks a list of rank-R tf.Tensors into one rank-(R+1) tf.Tensor.

Done.

tfjs-converter/src/executor/tensor_list.ts, line 78 at r2 (raw file):

Previously, lina128 (Na Li) wrote…

If unequal always result in an error, then why do we pass in the value? Can we just use this.elementDtype?

this is a parameter from the TF op.

tfjs-converter/src/executor/tensor_list.ts, line 192 at r2 (raw file):

Previously, lina128 (Na Li) wrote…

Return selected tensors in the TensorList as a stacked tensor. All of selected tensors ...

Done.

tfjs-converter/src/executor/tensor_list.ts, line 208 at r2 (raw file):

Previously, lina128 (Na Li) wrote…

Can we add an annotation why we need to do this? It is not obvious from the code.

Done.

tfjs-converter/src/executor/tensor_list.ts, line 235 at r2 (raw file):

Previously, lina128 (Na Li) wrote…

Why the shape is not just [0]?

It need to conform the requested element shape.

tfjs-converter/src/executor/tensor_list.ts, line 270 at r2 (raw file):

Previously, lina128 (Na Li) wrote…

Capitalize.

Done.

tfjs-converter/src/executor/tensor_list.ts, line 282 at r2 (raw file):

Previously, lina128 (Na Li) wrote…

Put tensors at specific indices of a stacked tensor into a TensorList.

Done.

tfjs-converter/src/executor/tensor_list.ts, line 313 at r2 (raw file):

Previously, lina128 (Na Li) wrote…

into a

Done.

tfjs-converter/src/executor/tensor_list.ts, line 314 at r2 (raw file):

Previously, lina128 (Na Li) wrote…

remove with?

Done.

tfjs-converter/src/executor/tensor_list.ts, line 316 at r2 (raw file):

Previously, lina128 (Na Li) wrote…

Remove Tensor, ?

Done.

lina128 approved these changes

View reviewed changes

Collaborator

lina128 left a comment

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

pyu10055 merged commit e190411 into master

pyu10055 deleted the tensor_list branch

June 9, 2020 00:51

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels