Skip to content

fix: Use non-traditional RoPE in Qwen2 test case.#56

Merged
skyzh merged 1 commit intoskyzh:mainfrom
jiengup:fix-use-traditional
Sep 7, 2025
Merged

fix: Use non-traditional RoPE in Qwen2 test case.#56
skyzh merged 1 commit intoskyzh:mainfrom
jiengup:fix-use-traditional

Conversation

@jiengup
Copy link
Copy Markdown
Contributor

@jiengup jiengup commented Sep 4, 2025

Since you claim that "The Qwen2 model uses a non-traditional form of RoPE" in the tutorial book, maybe it's better to use a non-traditional RoPE test case and notify the followers.

@skyzh skyzh merged commit bf3383d into skyzh:main Sep 7, 2025
1 check passed
jiengup added a commit to jiengup/tiny-llm that referenced this pull request Sep 8, 2025
@Sunjnn
Copy link
Copy Markdown

Sunjnn commented Sep 9, 2025

I don’t quite understand this change. Literally speaking, doesn’t rope_traditional=False mean non-traditional?

@jiengup jiengup deleted the fix-use-traditional branch September 9, 2025 13:52
jiengup added a commit to jiengup/tiny-llm that referenced this pull request Sep 9, 2025
@jiengup jiengup mentioned this pull request Sep 9, 2025
jiengup added a commit to jiengup/tiny-llm that referenced this pull request Sep 9, 2025
@jiengup
Copy link
Copy Markdown
Contributor Author

jiengup commented Sep 9, 2025

I don’t quite understand this change. Literally speaking, doesn’t rope_traditional=False mean non-traditional?

Sorry for misunderstand the original code. I've create aonther PR to revert this commit. #62
Thanks for point this out.

skyzh added a commit that referenced this pull request Sep 10, 2025
* Revert "fix: Use non-traditional RoPE in Qwen2 test case. (#56)"

This reverts commit bf3383d.

* Update week1-03-gqa.md with RoPE note and test command

Added note about using non-traditional RoPE and testing command.

---------

Co-authored-by: Alex Chi Z. <4198311+skyzh@users.noreply.github.com>
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.

3 participants