Pooling::mean divided the summed token embeddings by the total of the entire mask (the whole batch's tokens, times hidden size on the candle path) instead of by each row's own non-padding token count, which scales every pooled vector by a wrong, batch-dependent factor. This was not noticeable because final L2 norm nullifies the effect. If working with unnormalized vectors, output would be incorrectly scaled.
Demonstration of the issue here:
https://github.com/filament-dm/EmbedAnything/tree/mean-pool-fix
The first commit in that branch, filament-dm@7cb0f18, produces tests that fail due to the mean pool bug.
The second commit, filament-dm@a0583f9, fixes the implementation.
I've put this out on a branch rather than a PR since this is a fairly significant change in my view and probably requires a bit of careful thought.
Pooling::mean divided the summed token embeddings by the total of the entire mask (the whole batch's tokens, times hidden size on the candle path) instead of by each row's own non-padding token count, which scales every pooled vector by a wrong, batch-dependent factor. This was not noticeable because final L2 norm nullifies the effect. If working with unnormalized vectors, output would be incorrectly scaled.
Demonstration of the issue here:
https://github.com/filament-dm/EmbedAnything/tree/mean-pool-fix
The first commit in that branch, filament-dm@7cb0f18, produces tests that fail due to the mean pool bug.
The second commit, filament-dm@a0583f9, fixes the implementation.
I've put this out on a branch rather than a PR since this is a fairly significant change in my view and probably requires a bit of careful thought.