-
Notifications
You must be signed in to change notification settings - Fork 613
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
Snake layer and activation to learn periodic functions #1967
Conversation
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 contribution! Generally LGTM.
|
||
|
||
@tf.keras.utils.register_keras_serializable(package="Addons") | ||
def snake(inputs: types.TensorLike, freq: types.Number = 1) -> tf.Tensor: |
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.
can we use the full name frequency
here?
tensorflow_addons/layers/snake.py
Outdated
""" | ||
|
||
@typechecked | ||
def __init__(self, freq_initializer: types.Initializer = "ones", **kwargs): |
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.
ditto
tensorflow_addons/layers/snake.py
Outdated
self.freq = self.add_weight(initializer=freq_initializer, trainable=True) | ||
|
||
def call(self, inputs): | ||
return inputs + (1 - tf.cos(2 * self.freq * inputs)) / (2 * self.freq) |
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.
can we directly call tensorflow_addons.activations.snake
here?
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.
It imposes additional overhead. That approach used in test degraded performance by 30%.
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.
that's weird. I don't think one convert_to_tensor
and cast
have such impact on performance. But anyway, because that's how core TF deals with activation and layer subclass, it'd be better to use tensorflow_addons.activations.snake
IMO.
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.
OK. They know the best.
import numpy as np | ||
|
||
from tensorflow_addons.activations import snake | ||
|
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: remove this empty line
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.
Sure. The reason I was doing so is I was trying to somehow group similar imports since there are no strict guidelines on how to order imports, nor other layers and activations adhere some consistent pattern.
|
||
from tensorflow_addons.layers.snake import Snake | ||
from tensorflow_addons.activations.snake import snake | ||
|
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.
ditto
@@ -22,3 +22,4 @@ | |||
from tensorflow_addons.activations.rrelu import rrelu | |||
from tensorflow_addons.activations.sparsemax import sparsemax | |||
from tensorflow_addons.activations.tanhshrink import tanhshrink | |||
from tensorflow_addons.activations.snake import snake |
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.
place in alphabetical order
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.
@WindQAQ @seanpmorgan Just as a reminder can we automate with https://pypi.org/project/flake8-isort/?
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.
Looks good to me. Could you open and issue and PR for it? I'm fine to automate it.
tensorflow_addons/layers/__init__.py
Outdated
@@ -36,3 +36,4 @@ | |||
from tensorflow_addons.layers.tlu import TLU | |||
from tensorflow_addons.layers.wrappers import WeightNormalization | |||
from tensorflow_addons.layers.esn import ESN | |||
from tensorflow_addons.layers.snake import Snake |
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.
ditto
"""Snake activation to learn periodic functions. | ||
|
||
https://arxiv.org/abs/2006.08195 | ||
""" |
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.
could we add the arguments doc here?
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 was trying, but the original academic paper is quite obscure about explanations. So it could has end-up with words like 'frequency is a frequency' and other obvious things such as 'input is an input'. If it is OK anyway, no problems, I'll add.
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.
Got it. Maybe we can have something like
inputs: Input tensor.
frequency: A scalar, frequency of the periodic part.
"""Snake layer to learn periodic functions with the 'frequency' param trainable. | ||
|
||
https://arxiv.org/abs/2006.08195 | ||
""" |
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.
could we add the arguments doc here?
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 put them near the init for an IDE autocomplete.
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.
Hi @failure-to-thrive, I review a paper again and see this sentence in page 4.
We also suggest the extention to make the
$a$ parameter becomes a learnable parameter
for each preactivation value.
IMO, it suggests frequency
to be with the same shape as the input (like PReLU). What do you think?
Also, following the layers with learnable parameter in core TF, we should also support some arguments like frequency_regularizer
and frequency_constraint
.
tensorflow_addons/layers/snake.py
Outdated
def __init__(self, frequency_initializer: types.Initializer = "ones", **kwargs): | ||
""" | ||
frequency_initializer: Initializer for the 'frequency' param. | ||
""" |
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.
Sorry for the confusion, after a rough look at our docs, it would be better to move this near class Snake
.
class Snake(tf.keras.layers.Layer):
"""Snake layer to learn periodic functions with the trainable `frequency` scalar.
It follows:
```
f(x) = x + (1 - cos(2 * frequency * x)) / (2 * frequency)
```
where `frequency` is a learned scalar.
See [Neural Networks Fail to Learn Periodic Functions and How to Fix It](https://arxiv.org/abs/2006.08195) for more details.
Arguments:
frequency_initializer: Initializer for the `frequency` scalar.
frequency_regularizer: Regularizer for the `frequency` scalar.
frequency_constraint: Constraint for the `frequency` scalar.
"""
I'm following doc of the parametric activation like PReLU here. Feel free to elaborate more.
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.
IMO, it suggests
frequency
to be with the same shape as the input (like PReLU). What do you think?
As I said earlier, the paper is quite obscure in many aspects. The same statement could have been made about the activation. Guesswork is a Sisyphus work. So I propose to leave it as it is. We can always get back to it later.
Also, following the layers with learnable parameter in core TF, we should also support some arguments like
frequency_regularizer
andfrequency_constraint
.
Same here. I thought of it but the paper does not give a glue on how these 'regularizers and co' should be applied. If they are relevant here at all.
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.
Got it :-) Then just move the doc under class Snake
.
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 had to eliminate the math formula because it looks very awkward in the plain text. In the end of the all, there is the academic paper an one must be in good knowledge before utilizing the 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.
ok
* Snake layer and activation to learn periodic functions. * Removed @tf.function * Considering all the comments. * Black matters :) * Moved the doc under class Snake.
* Snake layer and activation to learn periodic functions. * Removed @tf.function * Considering all the comments. * Black matters :) * Moved the doc under class Snake.
Closes #1939