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 StringLower/StringUpper op to support lower/upper() for string operations #25859
Add StringLower/StringUpper op to support lower/upper() for string operations #25859
Conversation
a68ff20
to
2f61c67
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need to specify a unicode encoding somewhere here since we're manipulating characters?
Thanks @alextp. I take a look at TF 2.0 API: https://www.tensorflow.org/versions/r2.0/api_docs/python/tf/strings It looks like unicode operations are separated with |
I'd prefer to only have unicode versions of these operations since the
unicode version can have a fast path for ascii strings.
…On Wed, Feb 27, 2019 at 11:27 AM Yong Tang ***@***.***> wrote:
Thanks @alextp <https://github.com/alextp>. I take a look at TF 2.0 API:
https://www.tensorflow.org/versions/r2.0/api_docs/python/tf/strings
It looks like unicode operations are separated with unicode_ prefixes.
Wondering if it make sense to create similar unicode_upper and
unicode_lower? Or we could also consolidate unicode operations into part
of the existing strings operation?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#25859 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAATxXhiy0zOTZgvhryYcutoGz6f3tUUks5vRtwsgaJpZM4bCAT4>
.
--
- Alex
|
Thanks @alextp. I will update the PR to add the unicode option on top of |
This fix tries to address the issue raised in 25857 where there is no lower() for string operations yet. This fix adds StringLower to the kernel to support this op. This fix fixes 25857. Signed-off-by: Yong Tang <yong.tang.github@outlook.com>
Signed-off-by: Yong Tang <yong.tang.github@outlook.com>
Signed-off-by: Yong Tang <yong.tang.github@outlook.com>
Signed-off-by: Yong Tang <yong.tang.github@outlook.com>
Signed-off-by: Yong Tang <yong.tang.github@outlook.com>
bazel-bin/tensorflow/tools/api/tests/api_compatibility_test --update_goldens True Signed-off-by: Yong Tang <yong.tang.github@outlook.com>
Signed-off-by: Yong Tang <yong.tang.github@outlook.com>
2f61c67
to
5ebe920
Compare
with additional encoding='' support, utf-8 is supported at the moment Signed-off-by: Yong Tang <yong.tang.github@outlook.com>
Signed-off-by: Yong Tang <yong.tang.github@outlook.com>
@alextp Thanks for the review and sorry for the delay. The PR has been updated to add an additional arg of |
Sweet |
@alextp Thank you for reviewing! Sorry that I tagged |
Oh I remove the API review tag because the API owners discussed and are
fine with it.
…On Thu, Apr 25, 2019 at 3:10 PM Penporn Koanantakool < ***@***.***> wrote:
@alextp <https://github.com/alextp> Thank you for reviewing! Sorry that I
tagged API review early. I wasn't sure if it could be tagged at any time
or just after the PR is approved so I kind of tagged it early. Will only
tag after PR is approved next time.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#25859 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAABHROC3MEWJ4WKZX4EBFLPSIT6LANCNFSM4GYIAT4A>
.
--
- Alex
|
@alextp Oh, I meant you ended up having to review the whole PR instead because I tagged |
@yongtang Could you please take a look at test failures? |
Signed-off-by: Yong Tang <yong.tang.github@outlook.com>
@penpornk Thanks for the review. The python 3 string error has been fixed and the PR has been updated. There are other test failures but it looks like they are unrelated. Please take a look and let me know if there are any issues. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The tests look fine now. Thanks again for your contribution! :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it was just some flaky tests. The internal tests are being rerun. I'll let you know if we need more edits. Thank you for asking!
This fix tries to address the issue raised in #25857 where there is no lower() or upper() for string operations yet.
This fix adds StringLower/StringUpper to the kernel to support this op.
The endpoints are exposed as
strings.lower
andstrings.upper
.This fix fixes #25857.
Signed-off-by: Yong Tang yong.tang.github@outlook.com