Skip to content

Added implementation for Scaled RoPE. - #261

Merged
awni merged 6 commits into
ml-explore:mainfrom
hazemessamm:main
Dec 31, 2023
Merged

awni merged 6 commits into
ml-explore:mainfrom
hazemessamm:main

Conversation

@hazemessamm

@hazemessamm hazemessamm commented Dec 22, 2023 •

Copy link
Copy Markdown
Contributor

Proposed changes

Reference: https://kaiokendev.github.io/context#a-bigger-problem

Checklist

Put an x in the boxes that apply.

  • I have read the CONTRIBUTING document
  • I have run pre-commit run --all-files to format my code / installed pre-commit prior to committing changes
  • I have added tests that prove my fix is effective or that my feature works
  • I have updated the necessary documentation (if needed)

@awni awni left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for adding this! I have a question about the use of scale. O/w this is nice and we should merge it 👍

):
D = D // 2
positions = mx.arange(offset, N, dtype=dtype)
positions = mx.arange(offset, N, dtype=dtype) * scale

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When I've seen this parameter before it usually is the denominator in a division rather than multiplied with. So you would set scale = 4.0 and then positions = positions * (1 / scale).

What do you think? Do you have examples where it is used?

@hazemessamm hazemessamm Dec 30, 2023 •

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with you if the user is passing integers, I assumed that the user will pass the scale fraction so that I can multiply it directly with the positions without the need to do (1 / scale). What do you think? Should I change the scale parameter to expect integers and do (1 / scale) before multiplying or just assume that the user will pass (1 / scale) to the scale parameter directly?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can keep it the way it is. Calling it a scale makes it sound like it directly multiplies the positions (rather than divides them), so from a naming perspective the way you have it makes sense.

@awni

awni commented Dec 31, 2023

Copy link
Copy Markdown
Member

Could you rebase this on main and resolve conflicts, then we can merge it?

@awni
awni merged commit e3b8da2 into ml-explore:main Dec 31, 2023
JayYun98 pushed a commit to JayYun98/mlx that referenced this pull request Jan 7, 2024
* Added scale for RoPE

* Ran pre-commit

* Added RoPE scaling test

* Added docstring for scale parameter

* Modified docstrings
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants