-
Notifications
You must be signed in to change notification settings - Fork 74.2k
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 custom_objects in the function layer_test #36273
Conversation
layer test for custom layer Usage: ``` testing_utils.layer_test( SyncBatchNormalization, # keras.layers.BatchNormalization, kwargs={ 'momentum': 0.9, 'epsilon': 0.1, 'gamma_regularizer': keras.regularizers.l2(0.01), 'beta_regularizer': keras.regularizers.l2(0.01) }, input_shape=(3, 4, 2), custom_objects={'SyncBatchNormalization': SyncBatchNormalization}) ```
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please visit https://cla.developers.google.com/ to sign. Once you've signed (or fixed any issues), please reply here with What to do if you already signed the CLAIndividual signers
Corporate signers
ℹ️ Googlers: Go here for more info. |
@googlebot I signed it! |
CLAs look good, thanks! ℹ️ Googlers: Go here for more info. |
Hi Mokke, Just realized I accidentally commented on the issue instead of this PR: layer_test is a totally internal method, we don't want external users relying on it. This change seems explicitly designed for external users of Keras implementing their own layers and trying to use layer_test. The layer_test utility isn't an effective test of layer behavior, it just makes sure certain things don't crash. You would be better served writing your own set of unit tests that actually verify layer behavior (and that make sure training, evaluation, etc. work correctly). That said, I'll add @yhliang2018 to see if she thinks that there are some sort of testing utilities we should be exposing to external users in Keras' API. |
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.
(See my above comment)
@MokkeMeguru Can you please check tomerk's comments and keep us posted? Thanks! |
I waited @yhliang2018 's comment. |
@@ -93,7 +93,9 @@ def layer_test(layer_cls, kwargs=None, input_shape=None, input_dtype=None, | |||
string or integer values. | |||
adapt_data: Optional data for an 'adapt' call. If None, adapt() will not | |||
be tested for this layer. This is only relevant for PreprocessingLayers. | |||
|
|||
custom_objects: Optional Objects for your custom layer. If you write a | |||
your layer, you can use this variable. |
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.
nit: custom_objects: Optional dictionary mapping name strings to custom objects in the layer class. This is helpful to test a custom layer.
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.
Thanks your help!
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 @yhliang2018 meant that you should update this description to instead say:
custom_objects: Optional dictionary mapping name strings to custom objects in the layer class. This is helpful for testing custom layers.
Then we can go ahead and approve this.
Thanks for the PR! I think it's okay to add an extra arg to |
response? Should I tell you anything more? |
@MokkeMeguru Can you please address the reviewers comments and resolve conflicts? Thanks! |
I fixed import error for organizing imports. |
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.
Thanks for the fix. LGTM.
I don't know why this pr is not accepted by Unity Sanity with pylint error. logs.
|
this means we should fix the code,
to
? (two space to four space) |
TensorFlow's linting requires four spaces.
I updated PR for passing your lint test. |
I don't know why windows build will be crash. |
I updated the description because I think the curly braces in the code snippets are causing issues with the merging. @gbaned how do we restart the merging? Usage:
|
Thanks a lot! |
Add a custom_objects argument to layer_test, to enable running layer_test for custom layers.
reference: #36272