Fix zero frequency index along the width in metrics.general.power_spectrum - #2008
Nicholas022400701 wants to merge 2 commits into
Conversation
…ctrum After fftshift the zero frequency of an axis of length n sits at index n // 2 for even and odd n alike. The radial wavenumber grid used h // 2 for the height but w / 2 for the width, so for an odd width every wavenumber along the width was off by half a cell. A cosine along the width then landed in different bins than the same cosine along the height and the spectrum of a field differed from the spectrum of its transpose. Build the grid in float32 and use w // 2 for the width. Signed-off-by: 区梓灏 <116372750+Nicholas022400701@users.noreply.github.com>
The same cosine along the height and along the width has to give the same azimuthally averaged spectrum, the spectrum of a field has to match the spectrum of its transpose and a single cosine along the width has to fill exactly one wavenumber bin. All three held for even sizes and failed for odd widths before the fix. Signed-off-by: 区梓灏 <116372750+Nicholas022400701@users.noreply.github.com>
CODEOWNERS review mapCurrent for commit ⏳ @dallasfoster — 1 file(s)
⏳ @NickGeneva — 1 file(s)
No CODEOWNER
Comment |
 The PR appears safe to merge, with the numerical correction matching post-FFT-shift indexing and targeted tests covering the regression. SummaryCorrects the radial wavenumber grid so the width axis uses the actual post-
Reviews (1) · Last reviewed commit: "Test power_spectrum on odd sized grids" |
|
Update (2026-09-30): I reran the focused CPU reproducer against current I also reran the same control against this PR's head This is a focused reproducer only; I did not run the full PhysicsNeMo suite. The benchmark currently uses even grid sizes, so these results do not alter its measured spectra. This update confirms the tracked defect remains in main and that the existing PR addresses this reproducer; it is not a duplicate issue or a broader validation claim. |
PhysicsNeMo Pull Request
Description
closes #2007
power_spectrumbuilds the radial wavenumber grid afterfftshiftwithh // 2for the height andw / 2for the width. Afterfftshiftthe zero frequency of an axis of length n sits at indexn // 2for even and odd n alike, so for an odd width the whole grid was skewed by half a cell along the width. A cosine along the height and the same cosine along the width gave different spectra, the spectrum of a field did not match the spectrum of its transpose and a single mode along the width filled two bins instead of one. Even widths were not affected, which is why the existingtest_power_spectrumon a 32 by 32 grid never caught it.Changes:
meshgridcoordinates in float32 and usew // 2for the width, so both axes are centered the same way.torch.hypothas no integer kernel, so the plain//change alone would raise. The trailing.to(torch.float32)is gone because the result already is float32.test/metrics/test_power_spectrum_center.pywith three checks on 32 and 33 sized grids. The same cosine along either axis gives the same spectrum, the spectrum is invariant under transpose for 32 by 32, 33 by 33 and 32 by 33 fields, and a single cosine along the width of a 33 by 33 grid fills exactly one bin centered within half a bin width of the true wavenumber.Nothing else in the file changed.
Verification on a CPU GitHub Actions runner with torch CPU wheels and Python 3.12:
test_power_spectrumpass,ruff format --checkandruff checkare clean on both files,test/ci_tests/check_docstring_coverage.pyandtest/ci_tests/header_check.pypass. https://github.com/Nicholas022400701/oss-ci/actions/runs/35357928446I did not touch CHANGELOG.md in this PR. I can add a line under Fixed in the 2.3.0 section if you want it here.
AI disclosure: I used an AI coding agent to help write this patch, the tests and this description. I have read the change and the tests myself and I will answer review comments personally.
Checklist
Dependencies
None.
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.