-
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
Add tf.nn.gelu alias for TF >= 2.4 #2265
Conversation
You are owner of some files modified in this pull request. |
Mentioning @seanpmorgan . |
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 very good thanks! Mind just modifying the version check to be consistent with elsewhere in the repo:
https://github.com/tensorflow/addons/blob/master/tensorflow_addons/losses/tests/quantiles_test.py#L88
@@ -71,7 +72,9 @@ def gelu(x: types.TensorLike, approximate: bool = True) -> tf.Tensor: | |||
|
|||
x = tf.convert_to_tensor(x) | |||
|
|||
return _gelu_py(x, approximate) | |||
gelu_op = tf.nn.gelu if LooseVersion(tf.__version__) >= "2.4" else _gelu_py |
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.
We should also warn users that the default value of approximate
is changed from True
to False
if tf.nn.gelu
is being used. Like
warnings.warn("Default value of `approximate` is changed from `True` to `False`")
Thanks! Marking blocked since this should be tested with #2250 . It shouldn't be long before the release now. Also please add the Warning Tzu-Wei suggested and then should be good. |
Added a warning in the docstring as well. |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Hmm very slight difference w/ the keras implementation:
Is there anyway to increase doc test tolerance? Perhaps we just get rid of this doctest since gelu is on its way out? @WindQAQ |
I think we can just change the value of doctest. Actually, old c++ impl also gets different values from pure python ops. There is slight difference in casting and constant precision between c++ impl/python in addons/python in tf.nn.gelu. |
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.
LGTM thanks again @aaronmondal !
* Add tf.nn.gelu alias for TF >= 2.4
Fixes #2252
See discussion in the above mentioned issue and the related, dropped PR #2256