Skip to content

Conversation

@joyalbin
Copy link

@joyalbin joyalbin commented Aug 3, 2018

This PR is to remove the code duplication in tensoflow testcases

@srkreddy1238
Copy link
Contributor

Suggest to wait a while until #1505 is merged.
Above PR offers unified function which can be used along with local optimizations this PR offers.

@tqchen
Copy link
Member

tqchen commented Aug 4, 2018

please resync the changes against current master as in #1394

return output_data


def compate_tf_with_tvm(in_data, in_name, out_name, init_global_variables=False):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

typo compate

[tf_out[0].shape, (2, batch_size, num_hidden)],
[tf_out[0].dtype, tf_out[1].dtype])

if isinstance(tvm_output, list):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

assert tvm_output is list?

@yzhliu yzhliu added status: review in progress status: need update need update based on feedbacks labels Aug 6, 2018
@yzhliu yzhliu self-assigned this Aug 6, 2018
@srkreddy1238
Copy link
Contributor

@joyalbin thanks for improving the test cases.

Please note using #1505 we could improve further with common check_function API.
This can be handled outside this PR once the other is merged.

@joyalbin
Copy link
Author

joyalbin commented Aug 7, 2018

@srkreddy1238 @yzhliu I have handled the review comments and rebased the PR,
please help for further review

@joyalbin joyalbin force-pushed the tf_test_opt branch 2 times, most recently from 1aad10c to 0ecd582 Compare August 9, 2018 04:33
Copy link
Contributor

@srkreddy1238 srkreddy1238 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @joyalbin for converting Constants to Placeholders.

Please handle the comment, otherwise LGTM.

test_forward_multi_input()
test_forward_inception_v3()
test_forward_inception_v1()
#test_forward_inception_v1()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pls remove local changes.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@yzhliu
Copy link
Member

yzhliu commented Aug 10, 2018

@srkreddy1238 would you mind take another look and approve if it is good to you?
@joyalbin Please merge from master again.

Copy link
Contributor

@srkreddy1238 srkreddy1238 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM after rebase and CI pass.

@yzhliu yzhliu merged commit 84296f9 into apache:master Aug 13, 2018
@yzhliu
Copy link
Member

yzhliu commented Aug 13, 2018

Thanks, this is now merged.

FrozenGene pushed a commit to FrozenGene/tvm that referenced this pull request Dec 27, 2018
* [NNVM][TENSORFLOW] Optimized tensorflow testcases

* Replace Constants with Placeholder

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants