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.

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

Clean all those safe_div, _safe_div methods #21798

Merged
merged 6 commits into from Sep 24, 2018
Merged

Clean all those safe_div, _safe_div methods #21798

merged 6 commits into from Sep 24, 2018

Conversation

facaiy
Copy link
Member

@facaiy facaiy commented Aug 22, 2018 •
edited

tf.div_no_nan was introduced by #21621, which is designed to replace all those safe_div methods here.

Ref: #21784

@martinwicke please add a API design label, because we add a new argument: negative_to_zero.

@facaiy facaiy force-pushed the ENH/div_no_nan_treate_negative_as_zero branch from ff007af to c05bb4e Compare August 22, 2018 15:10
martinwicke
martinwicke reviewed Aug 22, 2018
View reviewed changes
tensorflow/contrib/losses/python/losses/loss_ops.py Outdated
@@ -104,7 +78,8 @@ def _safe_mean(losses, num_present):
then zero is returned.
"""
total_loss = math_ops.reduce_sum(losses)
return _safe_div(total_loss, num_present)
return math_ops.div_no_nan(total_loss, num_present,
negative_to_zero=True, name="value")
Copy link
Member

Choose a reason for hiding this comment

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

I'd much prefer to replace div_no_nan(x, y, negative_to_zero=True, ...) with div_no_nan(x, tf.max(y, 0), ...) and not add the negative_to_zero argument, which I doesn't look to me like it makes things simpler.

Copy link
Member Author

Choose a reason for hiding this comment

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

Reasonable. I'd like to revise code as suggested later.

@facaiy facaiy force-pushed the ENH/div_no_nan_treate_negative_as_zero branch from 01caf86 to dd634a2 Compare August 23, 2018 08:32
@facaiy
Copy link
Member Author

facaiy commented Aug 23, 2018

@martinwicke negative_to_zero was removed.

facaiy
facaiy commented Aug 23, 2018
View reviewed changes
tensorflow/contrib/losses/python/losses/loss_ops.py Outdated
@@ -104,7 +78,9 @@ def _safe_mean(losses, num_present):
then zero is returned.
"""
total_loss = math_ops.reduce_sum(losses)
return _safe_div(total_loss, num_present)
return math_ops.div_no_nan(total_loss,
math_ops.maximum(num_present, 0),
Copy link
Member Author

Choose a reason for hiding this comment

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

I think maximum is unnecessary op here, because num_present should be non-negative in practice.

Copy link
Contributor

Choose a reason for hiding this comment

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

Does something break when you remove the maximum? This could be another case of us needing the double-tap-where trick to avoid NaNs in the gradient.

Copy link
Member Author

Choose a reason for hiding this comment

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

Hi, I don't remove the maximum. Test failed after I resolved the conflict with the lastest master. I'll tell you when I fix it later. :-)

@facaiy facaiy force-pushed the ENH/div_no_nan_treate_negative_as_zero branch from dd634a2 to 38f8110 Compare August 23, 2018 09:07
@facaiy
Copy link
Member Author

facaiy commented Sep 4, 2018

@akanimax @martinwicke could you comment or reassign? Thanks.

@facaiy
Copy link
Member Author

facaiy commented Sep 11, 2018

Hey, what am I doing wrong? It seems that both reviewers are selected by github automatically.

@facaiy facaiy force-pushed the ENH/div_no_nan_treate_negative_as_zero branch from 663cd26 to e9cfe88 Compare September 11, 2018 13:47
@facaiy facaiy force-pushed the ENH/div_no_nan_treate_negative_as_zero branch from e9cfe88 to 2dd5fb6 Compare September 12, 2018 06:32
@facaiy
Copy link
Member Author

facaiy commented Sep 12, 2018

@alextp Hi, I think all tests pass, and the failures are unrelated.

@alextp alextp added awaiting testing (then merge) kokoro:force-run Tests on submitted change ready to pull PR ready for merge process labels Sep 12, 2018
@kokoro-team kokoro-team removed the kokoro:force-run Tests on submitted change label Sep 12, 2018
@facaiy
Copy link
Member Author

facaiy commented Sep 21, 2018

Hi, any update here? thanks

@alextp alextp added the kokoro:force-run Tests on submitted change label Sep 24, 2018
@kokoro-team kokoro-team removed the kokoro:force-run Tests on submitted change label Sep 24, 2018
yongtang pushed a commit to yongtang/tensorflow that referenced this pull request Sep 24, 2018
@tensorflower-gardener
Merge pull request tensorflow#21798 from facaiy:ENH/div_no_nan_treate…
0c48c70 
…_negative_as_zero

PiperOrigin-RevId: 214290400
@tensorflow-copybara tensorflow-copybara merged commit e3c334e into tensorflow:master Sep 24, 2018
@facaiy facaiy deleted the ENH/div_no_nan_treate_negative_as_zero branch September 24, 2018 22:48
tensorflow-copybara pushed a commit that referenced this pull request Sep 24, 2018
@alextp @tensorflower-gardener
BEGIN_PUBLIC
6c40bc7 
Temporary rollback to fix forward compatibility.
END_PUBLIC

Automated rollback of commit 0c48c70. Revert #21798.

PiperOrigin-RevId: 214349479
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@martinwicke martinwicke martinwicke left review comments

@alextp alextp alextp approved these changes

@ispirmustafa ispirmustafa Awaiting requested review from ispirmustafa

Assignees

@akshaym akshaym

Labels
cla: yes ready to pull PR ready for merge process
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@facaiy @alextp @martinwicke @akshaym @googlebot @kokoro-team @tensorflow-copybara