-
Notifications
You must be signed in to change notification settings - Fork 19.5k
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
Clarification: new mean option is synonym with sum_over_batch_size in loss function base class #20352
Conversation
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
Undid ruff format while keeping typo fixes
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #20352 +/- ##
=======================================
Coverage 78.87% 78.87%
=======================================
Files 512 512
Lines 49264 49264
Branches 7953 7953
=======================================
Hits 38859 38859
Misses 8546 8546
Partials 1859 1859
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
keras/src/losses/loss.py
Outdated
@@ -132,7 +133,7 @@ def reduce_values(values, reduction="sum_over_batch_size"): | |||
): | |||
return values | |||
loss = ops.sum(values) | |||
if reduction == "sum_over_batch_size": | |||
if (reduction == "sum_over_batch_size") or (reduction == "mean"): |
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.
if reduction in ("mean", "sum_over_batch_size"):
keras/src/losses/loss.py
Outdated
@@ -177,7 +178,7 @@ def apply_mask(sample_weight, mask, dtype, reduction): | |||
"""Applies any mask on predictions to sample weights.""" | |||
if mask is not None: | |||
mask = ops.cast(mask, dtype=dtype) | |||
if reduction == "sum_over_batch_size": | |||
if (reduction == "sum_over_batch_size") or (reduction == "mean"): |
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.
Same here
Since
sum_over_batch_size
actually ismean
,mean
option was added as equivalent ofsum_over_batch_size
to explicitly and clearly state whatsum_over_batch_size
does, under the expected namemean
, while keepingsum_over_batch_size
for backwards compatibility. See also #18818, where the problem with the current name is noted.Also, why is done like this:
if there's the clearer and shorter keras mean?
loss = ops.mean(values, axis=None, keepdims=False)
Please take it,
sum_over_batch_size
is confusing, I already implementedmean
when I realized it was already done under a different name.Edit: already undid formating while keeping the typo fixes see last commit: 19933d9