Skip to content
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.

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

Add tf.repeat support equivalent to numpy.repeat #26517

Closed
wants to merge 9 commits into from

Conversation

yongtang
Copy link
Member

@yongtang yongtang commented Mar 9, 2019

(Most of the credit goes to RaggedTensor author, couldn't find the GitHub handle but the related commit is bad5d1a)

This PR tries to address the issue raised in #8246 to have tf.repeat equivalent to numpy.repeat.

Multiple attempts have been made in the past. The previous PR #15224 add the support with C++ which was closed, as a python implementation relying on existing ops is more desirable.

Now repeat has been added in ragged_util.py for RaggedTensor (the axis=None is not covered but this is fairly straight ward). The related commit is bad5d1a.

This PR

  • moves the repeat implementation in ragged_util.py to array_ops.py so that it is possible to get exposed.
  • adds additional logic to handle numpy.repeat with axis=None
  • expose repeat_with_axis in ragged_util.py again so that existing logic in ragged tensor does not break
  • expose tf.repeat in v1 and v2.

This fix fixes #8264.

Signed-off-by: Yong Tang yong.tang.github@outlook.com

@yongtang yongtang added kokoro:run kokoro:force-run Tests on submitted change labels Mar 10, 2019
@kokoro-team kokoro-team removed kokoro:run kokoro:force-run Tests on submitted change labels Mar 10, 2019
@rthadur rthadur requested a review from gunan March 11, 2019 20:34
@rthadur rthadur added this to Assigned Reviewer in PR Queue via automation Mar 11, 2019
@rthadur rthadur self-assigned this Mar 11, 2019
@rthadur rthadur added awaiting review Pull request awaiting review size:L CL Change Size: Large labels Mar 11, 2019
@gunan
Copy link
Contributor

gunan commented Mar 11, 2019

I would not be the right reviewer for this.
You also need to have a TF API owner review this change, I think

@gunan gunan removed their request for review March 11, 2019 22:13
@rthadur rthadur requested review from alextp and rmlarsen March 11, 2019 22:25
@alextp
Copy link
Contributor

alextp commented Mar 11, 2019

@seanpmorgan I think this belongs in addons for now. WDYT?

@tensorflowbutler tensorflowbutler removed the awaiting review Pull request awaiting review label Mar 12, 2019
@seanpmorgan
Copy link
Member

Initial Thoughts:

  1. Looks like a great contribution thanks @yongtang !
  2. I'm struggling to find a location for this in addons. A key aspect that we're trying to push is addons must match an API pattern that we support (e.g. layers, optimizers, etc.) or it belongs to a sub-package that a reputable contributor or organization maintains (e.g. seq2seq). Is there a sub-package we could imagine this living under? It seems like a primative tensor transformation... so unless we make a tfa.transform or something it might be a little tough.

@karmel @facaiy thoughts?

@alextp
Copy link
Contributor

alextp commented Mar 15, 2019

@seanpmorgan should this be tfa.manip.repeat so it can conceivably end up in tf.manip.repeat?

@facaiy
Copy link
Member

facaiy commented Mar 16, 2019

Looks like a great contribution

+1, good job!

Is there a sub-package we could imagine this living under? It seems like a primative tensor transformation... so unless we make a tfa.transform or something it might be a little tough.

Yes, we need to create a new sub-package if we think addons should accept the contribution. But I'm not sure if it's appropriate to put primitive ops in addons, because the requirements in https://github.com/tensorflow/addons are:

contributions that conform to well-established API patterns, but implement new functionality not available in core TensorFlow.

@karmel @dynamicwebpaige What do you think?

@seanpmorgan
Copy link
Member

Yeah, my high level thought is that tfa.manip may not be an easily maintainable subpackage as it could be hard to define that expected API structure. Not impossible though and I'd be open to a subpackage submission (Currently working on the process of doing such a thing).

Apologies for the tepid answer, but obviously Addons will be in a forever battle with scope creep.

@karmel
Copy link

karmel commented Mar 19, 2019

Maybe tf-text? I agree that it's not obvious that this fits inside the addons paradigm. Although, if it were an op for repeating with a python endpoint rather than python for the same, I guess that would be allowed.

@rthadur
Copy link
Contributor

rthadur commented May 16, 2019

@yongtang gentle ping to check review comments.

@rmlarsen
Copy link
Member

rmlarsen commented Jun 3, 2019

@yongtang could you please rebase?

@rthadur rthadur added ready to pull PR ready for merge process and removed ready to pull PR ready for merge process labels Jul 12, 2019
@rthadur rthadur added ready to pull PR ready for merge process and removed ready to pull PR ready for merge process labels Aug 13, 2019
(Most of the credit goes to RaggedTensor author,
couldn't find the GitHub handle but the related commit
is bad5d1a)

This PR tries to address the issue raised in 8246
to have tf.repeat equivalent to numpy.repeat.

Multiple attempts have been made in the past.
My previous PR 15224 add the support with C++ which
was closed, as a python implementation relying on existing
ops is more desireable.

This PR
- moves the repeat implementation in ragged_util.py
to array_ops.py so that it is possible to get exposed.

Signed-off-by: Yong Tang <yong.tang.github@outlook.com>
Signed-off-by: Yong Tang <yong.tang.github@outlook.com>
to comform to numpy.repeat

Signed-off-by: Yong Tang <yong.tang.github@outlook.com>
Signed-off-by: Yong Tang <yong.tang.github@outlook.com>
… test cases

Signed-off-by: Yong Tang <yong.tang.github@outlook.com>
and update api golden

Signed-off-by: Yong Tang <yong.tang.github@outlook.com>
Signed-off-by: Yong Tang <yong.tang.github@outlook.com>
…review feedback

Signed-off-by: Yong Tang <yong.tang.github@outlook.com>
PR Queue automation moved this from Approved by Reviewer to Reviewer Requested Changes Aug 18, 2019
Signed-off-by: Yong Tang <yong.tang.github@outlook.com>
@yongtang
Copy link
Member Author

@rthadur @alextp Previously, Kokoro CI test failed because of some API change in test_util.TensorFlowTestCase.

I have fixed the build failure and all tests passes now, please take a look.

Sorry for not catching the test failure earlier.

PR Queue automation moved this from Reviewer Requested Changes to Approved by Reviewer Aug 19, 2019
@tensorflow-bot tensorflow-bot bot added the kokoro:force-run Tests on submitted change label Aug 19, 2019
@kokoro-team kokoro-team removed the kokoro:force-run Tests on submitted change label Aug 19, 2019
@rthadur rthadur added ready to pull PR ready for merge process and removed ready to pull PR ready for merge process labels Aug 19, 2019
@rthadur
Copy link
Contributor

rthadur commented Aug 19, 2019

Merged internally , waiting for auto merge to happen.

tensorflow-copybara pushed a commit that referenced this pull request Aug 20, 2019
@mellanox-github
Copy link

Can one of the admins verify this patch?

@yongtang
Copy link
Member Author

Looks like the PR has been merged into master so I will close this PR. Thanks everyone for the help to make this happen 👍 ❤️ 🎉 !

@yongtang yongtang closed this Aug 20, 2019
PR Queue automation moved this from Approved by Reviewer to Closed/Rejected Aug 20, 2019
@yongtang yongtang deleted the 8264-repeat-tagged branch May 4, 2020 19:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
API review API Review cla: yes ready to pull PR ready for merge process size:L CL Change Size: Large
Projects
PR Queue
  
Closed/Rejected
Development

Successfully merging this pull request may close these issues.

Auto-Configuration Error: Cannot find cudnn.h at /usr/lib/x86_64-linux-gnu/include/cudnn.h