Fix bugged gradients when combiner == 'MEAN' #2505
Conversation
|
Wait hang on, I'm dumb. Fixing the PR! |
| weights = tf.ones_like(indices, dtype=params.dtype) / tf.cast( | ||
| tf.shape(indices)[1], params.dtype | ||
| ) | ||
| combiner = "sum" |
There was a problem hiding this comment.
Why we have this combiner overriding?
There was a problem hiding this comment.
It was a quick workaround, but the output and grads are correct, and performance is fine! Since combiner == "mean" does not support weights, we can get the right results by creating a dummy weight array (we do that anyway), then scaling it so that we get an unweighted mean instead of an unweighted sum.
There was a problem hiding this comment.
Maybe you should call tf.reduce_mean after custom op instead of integrating with it or combiner overriding.
|
The GPU test is failing. |
|
lol. This one is just a simple issue - the tolerances for the equality comparison are too tight for the float16 tests. Give me a sec and I'll fix it. |
|
Combiner is not used in , which is different with cpu custom op. Maybe you should call tf.reduce_mean after custom op instead of integrating with it.
|
| @@ -49,8 +49,13 @@ def _embedding_bag( | |||
| Returns: | |||
| A `Tensor` of the format specified by `data_format`. | |||
| """ | |||
| if weights is None: | |||
| if weights is None and combiner == "sum": | |||
There was a problem hiding this comment.
Use reduction instead of combiner
There was a problem hiding this comment.
The combiner is combining part of the layer and applies to the inputs/weights, not the output/loss! Is reduction still the right approach there?
There was a problem hiding this comment.
Could you add combiner document?
I can't see ubuntu gpu CI report in the PR, could you check it? |
Yeah landed on master (Shown by badge on central README): https://source.cloud.google.com/results/invocations/5a171fa3-3bad-4859-9a05-414e4c155851 |
How could I check it in the review process? Should I record the url? I could get the info in previous PRs. |
|
Hi, thanks for merging, and sorry for my slow replies! I've only been able to get to this at the weekends, but I am committed to maintaining it - it's unfortunate that we had to rush because of the broken CI, but I do think this PR should resolve most of the issues, and I see the follow-up PR is already open! |
Appreciate the maintainership @Rocketknight1 ! Its volunteer work so no need to apologize for slow responses :) |
Fixes #2502