-
Notifications
You must be signed in to change notification settings - Fork 487
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
fix gelu grad multi definitions error #1406
Conversation
deepmd/common.py
Outdated
except AttributeError: | ||
gelu = op_module.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.
tf.nn.gelu
v.s. op_module.gelu
, which is faster?
If the latter is faster can we use it by default?
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.
The later is indeed faster, but sometimes an error will be reported during a long MD process, we need to be more careful.
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.
Maybe we could follow the implementations of tensorflow(without approximate) and do some more tests to figure out the reasons of this particular error.
Isn't it a breaking change from approximate to non-approximate? Also, the behavior will differ among different TensorFlow versions. |
Co-authored-by: Jinzhe Zeng <jinzhe.zeng@rutgers.edu>
Codecov Report
@@ Coverage Diff @@
## devel #1406 +/- ##
==========================================
+ Coverage 74.42% 75.71% +1.28%
==========================================
Files 92 92
Lines 7507 7638 +131
==========================================
+ Hits 5587 5783 +196
+ Misses 1920 1855 -65
Continue to review full report at Codecov.
|
Agree, We use the approximate version of gelu, then we check the gelu errors later in another issue. Let's fix the multi definitions of gelu grad first. |
#1380 should be fixed by this PR.