Skip to content

Fixed sinusoidal positional embeddings formula#1554

Merged
CharlelieLrt merged 1 commit intoNVIDIA:mainfrom
CharlelieLrt:bugfix_positional_encodings
Apr 8, 2026
Merged

Fixed sinusoidal positional embeddings formula#1554
CharlelieLrt merged 1 commit intoNVIDIA:mainfrom
CharlelieLrt:bugfix_positional_encodings

Conversation

@CharlelieLrt
Copy link
Copy Markdown
Collaborator

PhysicsNeMo Pull Request

Description

Closes #1522 .

Checklist

Dependencies

Review Process

All PRs are reviewed by the PhysicsNeMo team before merging.

Depending on which files are changed, GitHub may automatically assign a maintainer for review.

We are also testing AI-based code review tools (e.g., Greptile), which may add automated comments with a confidence score.
This score reflects the AI’s assessment of merge readiness and is not a qualitative judgment of your work, nor is
it an indication that the PR will be accepted / rejected.

AI-generated feedback should be reviewed critically for usefulness.
You are not required to respond to every AI comment, but they are intended to help both authors and reviewers.
Please react to Greptile comments with 👍 or 👎 to provide feedback on their accuracy.

Signed-off-by: Charlelie Laurent <claurent@nvidia.com>
@CharlelieLrt
Copy link
Copy Markdown
Collaborator Author

/blossom-ci

@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps Bot commented Apr 8, 2026

Greptile Summary

This PR fixes a bug in the sinusoidal positional-embedding formula (issue #1522) where 2.0 ** np.linspace(0.0, num_freq, num=num_freq) produced non-octave frequency bands instead of the intended 2.0 ** np.arange(num_freq).

  • song_unet.py handles this correctly: the old sinusoidal path is preserved with a FutureWarning for backward compat, and a new sinusoidal_octave type uses the corrected formula. Pre-trained checkpoints can still be loaded without silent divergence.
  • multi_diffusion/models.py does the opposite: the sinusoidal formula is changed in-place, with no deprecation warning and no new gridtype. Any pre-trained MultiDiffusionModel2D checkpoint loaded after this PR will silently compute different positional embeddings than it was trained with.

Vulnerabilities

No security concerns identified.

Important Files Changed

Filename Overview
physicsnemo/diffusion/multi_diffusion/models.py Directly replaces the legacy np.linspace freq-band formula with np.arange, silently breaking backward compat for pre-trained checkpoints — unlike song_unet.py which adds sinusoidal_octave and deprecates the old path.
physicsnemo/models/diffusion_unets/song_unet.py Adds sinusoidal_octave gridtype with correct 2.0 ** np.arange(num_freq) freq bands; keeps old sinusoidal for backward compat and emits FutureWarning on both N_grid_channels == 4 and general paths.
test/models/diffusion/test_song_unet_pos_embd.py Adds test_sinusoidal_octave_freq_bands and test_sinusoidal_octave_differs_from_legacy; extends test_fails_if_grid_is_invalid with sinusoidal_octave; no tests for the N_grid_channels=4 edge case with sinusoidal_octave.

Reviews (1): Last reviewed commit: "Fixed sinusoidal positional embeddings f..." | Re-trigger Greptile

Comment thread physicsnemo/diffusion/multi_diffusion/models.py
@CharlelieLrt
Copy link
Copy Markdown
Collaborator Author

/blossom-ci

@CharlelieLrt CharlelieLrt added this pull request to the merge queue Apr 8, 2026
Merged via the queue into NVIDIA:main with commit 6c8f6f9 Apr 8, 2026
4 checks passed
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.

🐛[BUG]: Sinusoidal positional embedding freq_bands formula does not produce octave doublings

2 participants