-
Notifications
You must be signed in to change notification settings - Fork 127
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
Do not batch reduce as keras optimzer already does. #183
Conversation
Did you check for tf.compat.v1.train.optimzer? |
Correct me if I am wrong. I think this |
Sorry, you're right. And this bug seems may cause a problem if we don't assign specific local device when using keras model. |
done |
It does call batch_reduce_to twice on one call. |
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
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
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
Hi @acmore, thank you for the fix, and please squash the 3 commits into one, after that I will merge it. |
Thanks. I have squashed them into one. |
Description
Brief Description of the PR:
This pr is to remove
batch_reduce_to
call on the gradients for optimizer v2. In the optimzer v2 base class, the gradients are aggregated already.Type of change
Checklist:
How Has This Been Tested?
If you're adding a bugfix or new feature please describe the tests that you ran to verify your changes:
*