Skip to content

Add Addons namespace to Custom Ops #516

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
Merged
merged 12 commits into from
Sep 19, 2019
Merged

Add Addons namespace to Custom Ops #516

merged 12 commits into from
Sep 19, 2019

Conversation

tomerk
Copy link
Contributor

@tomerk tomerk commented Sep 17, 2019

Fixes issue #379
#379

This requires the nightly or the upcoming TF2 RC2, it does not work with the TF2 RC1

@seanpmorgan
Copy link
Member

Thanks very much Tomer this is a big help. Mind quickly formatting with:
bash tools/run_docker.sh -c 'make code-format'

@tomerk
Copy link
Contributor Author

tomerk commented Sep 17, 2019

Done!

WindQAQ
WindQAQ requested changes Sep 17, 2019
View reviewed changes
Copy link
Member

@WindQAQ WindQAQ left a comment •
edited
Loading

Choose a reason for hiding this comment

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

@tomerk Thanks for picking this up! Seems we miss hardshrink operation here (have to merge master though). Also, would you mind unifying all #ifndef, #define and #endif like TENSORFLOW_ADDONS_FOLDERNAME_KERNELS_OPNAME. I see some of them not follow the same style. Thanks!

qlzh727 and others added 6 commits September 17, 2019 14:29
@qlzh727
Port bug fix in TF contrib to addons. (tensorflow#497)
d74a967 
* Port bug fix in TF contrib to addons.

Original change at
tensorflow/tensorflow@a913689.

* Fix lint warning.
@ChenYang-ChenYang
check pass through and do the expand_dims() only if needed (tensorflo…
8887a23 
…w#464)

* check pass through and do the expand_dims() only if needed
* add indent to the fixed line
* merge return condition to if state
@WindQAQ
add hardshrink kernel (tensorflow#500)
92e61d6 
* add hardshrink kernel
* make linter happy
@kazemnejad
Fixing SequenceLoss Keras incompatibility (tensorflow#503)
d418747 
* Fix SequenceLoss incompatibility with Keras built-in loops

* Remove debugging prints

* Change the attribute existence checking to use more pythonic way
@guillaumekln
Replace some compat.v1 APIs by their v2 equivalent (tensorflow#507)
357667c 
* Replace some compat.v1 APIs by their v2 equivalent

* Fix lint error
@WindQAQ
Add documentation for LazyAdam (tensorflow#515)
2441300
@tomerk tomerk requested a review from Squadrick as a code owner September 17, 2019 21:29
@googlebot
Copy link

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

@tomerk
Copy link
Contributor Author

tomerk commented Sep 17, 2019

Done, but it looks like the way I rebased onto master messed up the CLA bot.

@seanpmorgan
Copy link
Member

Done, but it looks like the way I rebased onto master messed up the CLA bot.

No worries we can override CLA when time to merge.

@seanpmorgan
Merge branch 'master' into pr/tomerk/516
64b604f 
# Conflicts:
#	tensorflow_addons/activations/hardshrink.py
#	tensorflow_addons/custom_ops/activations/cc/kernels/hardshrink_op.cc
#	tensorflow_addons/custom_ops/activations/cc/kernels/hardshrink_op.h
#	tensorflow_addons/custom_ops/activations/cc/kernels/hardshrink_op_gpu.cu.cc
#	tensorflow_addons/custom_ops/activations/cc/ops/hardshrink_op.cc
@WindQAQ WindQAQ mentioned this pull request Sep 18, 2019
Merged
@facaiy
Copy link
Member

facaiy commented Sep 18, 2019

Nice job, Tomer!

facaiy
facaiy approved these changes Sep 19, 2019
View reviewed changes
Copy link
Member

@facaiy facaiy left a comment

Choose a reason for hiding this comment

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

Thank you, Tomer :-)

@facaiy
Copy link
Member

facaiy commented Sep 19, 2019

@ewilderj Hi, Edd, could you help override cla status? Thanks 😄

@facaiy
Copy link
Member

facaiy commented Sep 19, 2019

@WindQAQ @fsx950223 Hi, could you take another look? I'd like to merge it if you both agree :-)

@WindQAQ WindQAQ self-requested a review September 19, 2019 02:36
@facaiy facaiy added cla: yes and removed cla: no labels Sep 19, 2019
@googlebot
Copy link

☹️ Sorry, but only Googlers may change the label cla: yes.

@facaiy facaiy merged commit f4db0b1 into tensorflow:master Sep 19, 2019
@facaiy
Copy link
Member

facaiy commented Sep 19, 2019

Thanks, all!

@facaiy facaiy mentioned this pull request Sep 19, 2019
Closed
@ghost ghost mentioned this pull request Oct 4, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@facaiy facaiy facaiy approved these changes

@WindQAQ WindQAQ WindQAQ approved these changes

@fsx950223 fsx950223 fsx950223 approved these changes

@guillaumekln guillaumekln Awaiting requested review from guillaumekln guillaumekln is a code owner

@qlzh727 qlzh727 Awaiting requested review from qlzh727 qlzh727 is a code owner

@seanpmorgan seanpmorgan Awaiting requested review from seanpmorgan

@Squadrick Squadrick Awaiting requested review from Squadrick

Assignees

No one assigned

Labels

awaiting testing (then merge)

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.