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.

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

Fixed missing export for Huber loss as tf.keras.losses.huber and tf.keras.metrics.huber #37535

Closed
wants to merge 2 commits into from

Conversation

srihari-humbarwadi
Copy link
Contributor

No description provided.

@tensorflow-bot tensorflow-bot bot added the size:XS CL Change Size: Extra Small label Mar 12, 2020
@gbaned gbaned self-assigned this Mar 13, 2020
@gbaned gbaned added the comp:keras Keras related issues label Mar 13, 2020
@gbaned gbaned requested a review from pavithrasv March 13, 2020 04:10
@pavithrasv
Copy link
Member

Why is this required? Can we use the Huber loss class instead? The other loss functions are exported because they were available historically.

@srihari-humbarwadi
Copy link
Contributor Author

srihari-humbarwadi commented Mar 13, 2020

This would make the API consistent, since all the other loss function are available as classes and also as functions. And yes we can use Huber loss class just like the rest of the loss classes

@@ -1415,6 +1415,7 @@ def categorical_hinge(y_true, y_pred):
return math_ops.maximum(0., neg - pos + 1.)


@keras_export('keras.metrics.huber', 'keras.losses.huber')
Copy link
Member

Choose a reason for hiding this comment

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

Can we only add it to losses namespace for now? Also you will need to update golden files '.pbtxt' files for API changes.

Copy link
Contributor Author

@srihari-humbarwadi srihari-humbarwadi Mar 13, 2020

Choose a reason for hiding this comment

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

I have removed it from the tf.keras.metrics namespace, and also can you please let me know what .pbtxt files need changes?

@pavithrasv
Copy link
Member

@mihaimaruseac can you advice on how API changes ie.pbtxt files can be updated?

@mihaimaruseac mihaimaruseac added the API review API Review label Mar 13, 2020
@tensorflow-bot tensorflow-bot bot added kokoro:force-run Tests on submitted change ready to pull PR ready for merge process labels Mar 13, 2020
@kokoro-team kokoro-team removed the kokoro:force-run Tests on submitted change label Mar 13, 2020
@tensorflow-bot tensorflow-bot bot added the kokoro:force-run Tests on submitted change label Mar 13, 2020
@kokoro-team kokoro-team removed the kokoro:force-run Tests on submitted change label Mar 13, 2020
@mihaimaruseac
Copy link
Collaborator

To generate the proto files used for API you need to run the api_compatibility_test with --update_goldens:

bazel build tensorflow/tools/api/tests:api_compatibility_test
bazel-bin/tensorflow/tools/api/tests/api_compatibility_test --update_goldens=True

Then, this will need an API review (there are weekly meetings for this) and API approval before being imported

@gbaned gbaned added awaiting review Pull request awaiting review and removed ready to pull PR ready for merge process labels Mar 17, 2020
@alextp
Copy link
Contributor

alextp commented Mar 18, 2020

As tensorflow/api-owners: we'd rather keep Huber as class-only (as well any future losses).

@alextp alextp closed this Mar 18, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
API review API Review awaiting review Pull request awaiting review cla: yes comp:keras Keras related issues size:XS CL Change Size: Extra Small
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants