Some questions about the apply_multimodal_rotary_pos_emb function.
Author: Dyb3438
Pull request: no

Hi,

When reading the source code of Qwen2.5-VL, I found a potential issue in how the RoPE frequency embeddings are split across modalities inside apply_multimodal_rotary_pos_emb.

1. Correct behavior in frequency construction

In the code at lines 543–554, the RoPE frequencies are constructed as:

https://github.com/huggingface/transformers/blob/main/src/transformers/models/qwen2_5_vl/modeling_qwen2_5_vl.py#L543-L554


This duplicates the base frequency vector.
This part is correct.

2. Potential problem in apply_multimodal_rotary_pos_emb

Later, in apply_multimodal_rotary_pos_emb, the code splits the concatenated frequency vector directly:

https://github.com/huggingface/transformers/blob/ff13eb668aa03f151ded71636d723f2e490ad967/src/transformers/models/qwen2_5_vl/modeling_qwen2_5_vl.py#L607-L613


But because emb was built as [freq, freq], a direct split along the flattened last dimension may slice across the two duplicated halves, leading to mixed or “wrapped” frequency groups.

Example to illustrate the issue

Assume frequency list:

```
freq = [1/50, 1/60, 1/100, 1/200, 1/300]
emb = freq + freq
# emb = [f0, f1, f2, f3, f4,  f0, f1, f2, f3, f4]
```

If we split using:

mrope_section = [4,4,2]


The current code produces:
```
[
  [f0, f1, f2, f3],  
  [f4, f0, f1, f2],   
  [f3, f4]    
]
```

This mixes frequencies from the two duplicated halves, which seems unintended and inconsistent with RoPE’s paired structure.

3. Expected behavior

Because the duplication is meant to create frequency pairs, the correct way should preserve each (fi, fi) pair before splitting.

For example:
```
emb = emb.view(..., 2, 5).transpose(-1, -2)

so that it becomes:

[
  [f0, f0],
  [f1, f1],
  [f2, f2],
  [f3, f3],
  [f4, f4],
]
```

Then splitting should occur along the frequency axis:

mrope_section = [2, 2, 1]  # measured in freq-pairs, not flattened dim


After reconstruction, each chunk keeps its intended [fi, fi] structure, e.g.:
```
[
  [f0, f1, f0, f1], 
  [f2, f3, f2, f3], 
  [f4, f4]
]
```

This avoids wrap-around and matches the conceptual RoPE frequency grouping per modality.

4. Question

Is the current behavior intended?
Or should the split be applied after reshaping the [freq, freq] vector into explicit frequency pairs, so the modality-specific RoPE blocks are aligned correctly?

Comments:
Hmmn - that code was added in a PR from @ShuaiBai623 at Alibaba, so I have to assume it's correct , or at least that if it's not correct it matches the code used in Qwen training and we can't change it without breaking pretrained models 😅

cc @zucchini-nlp for viz, but not sure if it's worth a deep dive since it's unlikely to be fixable regardless!

Yep, since that is how RoPE was implemented in qwen repo and how the model was trained most probably, then changing it might incur quality degradation


@zucchini-nlp I realise that changing the released code is unrealistic now that the pretrained checkpoints have finished. However, I suspect that it could theoretically cause some issues with relative position learning in the context.

I didn't give it a deep dive yet but theoretically it might be incorrect indeed. I will check it out, in any case the authors of QwenVL will be the best point of contact for architecture questions

This issue has been automatically marked as stale because it has not had recent activity. If you think this still needs to be addressed please comment on this thread.

Please note that issues that do not follow the [contributing guidelines](https://github.com/huggingface/transformers/blob/main/CONTRIBUTING.md) are likely to be ignored.