Skip to content

[FRONTEND][TENSPRFLOW] Add split support #2001

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
Closed
nishi-t wants to merge 3 commits into from
Closed

[FRONTEND][TENSPRFLOW] Add split support #2001

nishi-t wants to merge 3 commits into from

Conversation

@nishi-t
Copy link
Contributor

@nishi-t nishi-t commented Oct 26, 2018

This PR adds following operators support to tensorflow frontend:

  • Split
  • SplitV

srkreddy1238
srkreddy1238 requested changes Oct 26, 2018
View reviewed changes
nnvm/tests/python/frontend/tensorflow/test_forward.py Outdated

# Relational ops
test_forward_rel_ops()

Copy link
Contributor

@srkreddy1238 srkreddy1238 Oct 26, 2018

Choose a reason for hiding this comment

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

Suggest to move under transforms.

nnvm/tests/python/frontend/tensorflow/test_forward.py Outdated
_test_forward_rel_op([t1, t2], math_ops.equal)
_test_forward_rel_op([t1, t2], math_ops.not_equal)

#######################################################################
Copy link
Contributor

@srkreddy1238 srkreddy1238 Oct 26, 2018

Choose a reason for hiding this comment

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

Pls add another test case with Split+Concat.

ghost
ghost reviewed Oct 26, 2018
View reviewed changes
nnvm/tests/python/frontend/tensorflow/test_forward.py Outdated
test_forward_reduce()
test_forward_mean()
#test_forward_reduce()
#test_forward_mean()
Copy link

@ghost ghost Oct 26, 2018

Choose a reason for hiding this comment

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

Any particular reason for commenting them?

Copy link

@ghost ghost Oct 26, 2018

Choose a reason for hiding this comment

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

Saw changes in your fix.

@tqchen tqchen added the status: need update need update based on feedbacks label Oct 27, 2018
@nishi-t nishi-t force-pushed the tf_split branch 3 times, most recently from 93ab1c2 to 3ef3ec0 Compare October 29, 2018 04:28
@nishi-t nishi-t mentioned this pull request Oct 29, 2018
Merged
srkreddy1238
srkreddy1238 approved these changes Oct 29, 2018
View reviewed changes
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 @nishi-t. LGTM.

srkreddy1238
srkreddy1238 requested changes Oct 29, 2018
View reviewed changes
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.

@nishi-t
#1572
You can take ref from this about how to handle split output.

@nishi-t
Copy link
Contributor Author

nishi-t commented Oct 29, 2018

Thank you for your review and approve, but the test case that concat after split currently is not passed. For now, I'll comment out this test case.

@nishi-t
Copy link
Contributor Author

nishi-t commented Oct 29, 2018 •
edited
Loading

@srkreddy1238 Thaks for the information. I'll check it out and work on the problem of handling split outputs.

@alexeyr alexeyr mentioned this pull request Nov 16, 2018
Merged
@alexeyr
Copy link
Contributor

alexeyr commented Nov 16, 2018

I think my pull request #2105 got SplitV correct. For your case it should be tuple(np.cumsum(indices)[:-1]).

@nishi-t
Copy link
Contributor Author

nishi-t commented Nov 16, 2018

Sorry, I couldn't work for this PR recently. I'll close my PR.
@alexeyr Thank you for submitting PR to support Split.

@nishi-t nishi-t closed this Nov 16, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@srkreddy1238 srkreddy1238 srkreddy1238 requested changes

+1 more reviewer
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

status: need update need update based on feedbacks

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@nishi-t @alexeyr @srkreddy1238 @tqchen