Use repeat_interleave for RoPE frequency expansion in rotate_queries_… #15
Conversation
…or_keys Previously, we used `.repeat(1,1,1,2)` to expand our RoPE sin/cos embeddings, which duplicated each frequency across its vector pair. This is a subtle bug. This change replaces that pattern with a direct `repeat_interleave(2, dim=-1)`, ensuring that each θᵢ is exactly duplicated for both dimensions of its 2×2 rotation block: - Before: `repeat(1,1,1,2)` - After: `repeat_interleave(2, dim=-1)` By interleaving correctly, we guarantee proper alignment of all coordinate pairs during the complex rotation.
|
Hi @echosprint! Thank you for your pull request and welcome to our community. Action RequiredIn order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you. ProcessIn order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA. Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with If you have received this in error or have any questions, please contact us at [email protected]. Thanks! |
|
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks! |
facebook-github-bot
added
the
CLA Signed
|
Hi @echosprint, Thanks for the catch! Indeed you are right that we use a different angle per coordinate axis in the current implementation. We can't merge the PR as this would break compatibility with the pretrained model but it would be useful to highlight the difference with RoPE with a comment. |
|
Echoing Nicolas, thanks for flagging! I added your proposed change (commented out) in #75 so others can easily make the change locally for their own pretraining if desired. Closing this PR out |
(facebookresearch#75) * Comments addressing PR facebookresearch#15 and Issue facebookresearch#65 * Link to PR facebookresearch#15
4 participants
Previously, we used
.repeat(1,1,1,2)to expand our RoPE sin/cos embeddings, which duplicated each frequency across its vector pair. This is a subtle bug.This change replaces that pattern with a direct
repeat_interleave(2, dim=-1), ensuring that each θᵢ is exactly duplicated for both dimensions of its 2×2 rotation block:repeat(1,1,1,2)repeat_interleave(2, dim=-1)By interleaving correctly, we guarantee proper alignment of all coordinate pairs during the complex rotation.