Skip to content

Conversation

@kangyizhang
Copy link
Contributor

@kangyizhang kangyizhang commented Dec 17, 2019

When SavedModel has one input multi outpus, it should return an array of output tensors.

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


This change is Reviewable

Copy link
Collaborator

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


tfjs-node/src/saved_model_test.ts, line 432 at r1 (raw file):

  });

  it('execute model with single inputs and outputs', async () => {

single input and multiple outputs?


tfjs-node/src/saved_model_test.ts, line 440 at r1 (raw file):

    const output1 = output[0];
    const output2 = output[1];
    expect(output1.shape).toEqual(input.shape);

this is test is bit confusing, why output is same as input? maybe states that this model is an identity op


tfjs-node/src/saved_model_test.ts, line 459 at r1 (raw file):

    const output1 = output['output_0'];
    const output2 = output['output_1'];
    expect(output1.shape).toEqual(input1.shape);

this old test seems to cover multiple outputs case, why it did not fail previously?

Copy link
Contributor Author

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


tfjs-node/src/saved_model_test.ts, line 432 at r1 (raw file):

Previously, pyu10055 (Ping Yu) wrote…

single input and multiple outputs?

done


tfjs-node/src/saved_model_test.ts, line 440 at r1 (raw file):

Previously, pyu10055 (Ping Yu) wrote…

this is test is bit confusing, why output is same as input? maybe states that this model is an identity op

added some comments explaining what the test model is doing


tfjs-node/src/saved_model_test.ts, line 459 at r1 (raw file):

Previously, pyu10055 (Ping Yu) wrote…

this old test seems to cover multiple outputs case, why it did not fail previously?

this old test covers model multiple inputs and multiple output. But if the model has single input and multiple outputs, when running the model with model.predict(input), the code will only return one output tensor. This PR fixes it.

Copy link
Collaborator

@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.

Reviewed 7 of 8 files at r1, 1 of 1 files at r2.
Reviewable status: :shipit: complete! 1 of 1 approvals obtained (waiting on @pyu10055)

@kangyizhang kangyizhang merged commit 6fdfe6d into master Dec 19, 2019
@kangyizhang kangyizhang deleted the node-output branch December 20, 2019 23:29
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