Skip to content

Conversation

@pyu10055
Copy link
Collaborator

@pyu10055 pyu10055 commented Jun 19, 2020

This will reduce tensor creation and possible tensor data read.

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


This change is Reviewable

@pyu10055 pyu10055 requested a review from lina128 June 19, 2020 20:06
Copy link
Collaborator

@lina128 lina128 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 @lina128 and @pyu10055)


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

    assertShapesMatchAllowUndefinedSize(
        elementShape, this.elementShape, 'TensorList shape mismatch: ');
    // return tidy(() => {

Remove.


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

    assertShapesMatchAllowUndefinedSize(
        tensor.shape, elementShape, 'TensorList shape mismatch: ');
    return tensor.reshape(elementShape);

Can we simply return tensor?


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

    }

    return tidy(() => {

Can we also remove the reshape and tidy here?


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

    }

    return tidy(() => {

Can we also remove the tidy and reshape?

Copy link
Collaborator Author

@pyu10055 pyu10055 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 @lina128)


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

Previously, lina128 (Na Li) wrote…

Remove.

I have reverted the change, I think we still need to do the reshape, since the elementShape is an input, we need to honor that.


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

Previously, lina128 (Na Li) wrote…

Can we simply return tensor?

same reason as above.


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

Previously, lina128 (Na Li) wrote…

Can we also remove the reshape and tidy here?

same reason as above.


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

Previously, lina128 (Na Li) wrote…

Can we also remove the tidy and reshape?

same.

Copy link
Collaborator

@lina128 lina128 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 @lina128 and @pyu10055)


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

Previously, pyu10055 (Ping Yu) wrote…

I have reverted the change, I think we still need to do the reshape, since the elementShape is an input, we need to honor that.

But you have asserted elementShape === this.elementShape. This is basically saying I only allow the elementShape to be the tensor's original elementShape. So below logic is correct, but is unnecessary?


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

   */
  constructor(
      readonly tensors: Tensor[], readonly elementShape: number[],

Should we check whether the tensors truly have the elementShape and elementDtype?

Copy link
Collaborator

@lina128 lina128 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 @lina128 and @pyu10055)


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

Previously, pyu10055 (Ping Yu) wrote…

same reason as above.

Either way, should we keep consistent with tensor_array.ts? https://github.com/tensorflow/tfjs/blob/master/tfjs-converter/src/executor/tensor_array.ts#L205

Copy link
Collaborator Author

@pyu10055 pyu10055 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 @lina128)


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

Previously, lina128 (Na Li) wrote…

But you have asserted elementShape === this.elementShape. This is basically saying I only allow the elementShape to be the tensor's original elementShape. So below logic is correct, but is unnecessary?

the check is compatible, they don't have to be exactly the same.


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

Previously, lina128 (Na Li) wrote…

Either way, should we keep consistent with tensor_array.ts? https://github.com/tensorflow/tfjs/blob/master/tfjs-converter/src/executor/tensor_array.ts#L205

TensorArray is slightly different, since there is no target tensor shape for any of these ops.


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

Previously, lina128 (Na Li) wrote…

Should we check whether the tensors truly have the elementShape and elementDtype?

good point.

Copy link
Collaborator

@lina128 lina128 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: :shipit: complete! 1 of 1 approvals obtained (waiting on @lina128 and @pyu10055)


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

Previously, pyu10055 (Ping Yu) wrote…

the check is compatible, they don't have to be exactly the same.

Ah, I see. So the original size may have -1, the passed in elementShape gives the actual shape. Thanks for the explanation.

@pyu10055 pyu10055 merged commit 3ca8d5c into master Jun 19, 2020
@pyu10055 pyu10055 deleted the op_fixes branch June 19, 2020 23:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants