Skip to content
This repository has been archived by the owner on Aug 15, 2019. It is now read-only.

Return 0 for tf.sign(NaN) to align with TF #998

Merged
merged 5 commits into from
Apr 25, 2018
Merged

Return 0 for tf.sign(NaN) to align with TF #998

merged 5 commits into from
Apr 25, 2018

Conversation

dsmilkov
Copy link
Contributor

@dsmilkov dsmilkov commented Apr 25, 2018 •
edited
Loading

This change is Reviewable

@dsmilkov dsmilkov requested a review from nsthorat April 25, 2018 19:36
@nsthorat
Copy link
Contributor

:lgtm_strong:

Review status: 0 of 1 files reviewed at latest revision, all discussions resolved, some commit checks failed.

Comments from Reviewable

@manrajgrover
Copy link
Contributor

@dsmilkov Isn't tf.sign(x) 0 for x == 0 or NaN?

https://www.tensorflow.org/api_docs/python/tf/sign

@dsmilkov dsmilkov changed the title NaN propagation in tf.sign is undefined. Don't test for that Return 0 for tf.sign(NaN) to align with TF Apr 25, 2018
@dsmilkov
Copy link
Contributor Author

Nice catch! Thanks. Fixing

manrajgrover
manrajgrover approved these changes Apr 25, 2018
View reviewed changes
Copy link
Contributor

@manrajgrover manrajgrover left a comment

Choose a reason for hiding this comment

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

LGTM 💯

@dsmilkov dsmilkov merged commit bf781a9 into master Apr 25, 2018
@dsmilkov dsmilkov deleted the sign-nan branch April 25, 2018 20:36
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@manrajgrover manrajgrover manrajgrover approved these changes

@nsthorat nsthorat Awaiting requested review from nsthorat

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@dsmilkov @nsthorat @manrajgrover