fix: norm scaling - #370
Open
jharlow-intel wants to merge 1 commit into
Open
Conversation
jharlow-intel
requested review from
antonwolfy,
ndgrigorian,
vlad-perevezentsev and
xaleryb
as code owners
August 26, 2026 21:26
Contributor
There was a problem hiding this comment.
Pull request overview
Fixes incorrect norm="forward" / "ortho" scaling in the root N-D FFT API (mkl_fft.fftn/ifftn/rfftn/irfftn and, via delegation, the fft2/ifft2/rfft2/irfft2 family) by computing the normalization basis from the transformed axes (and for irfftn from the complex-to-real output length along the last transformed axis), aligning behavior with the existing interface wrappers.
Changes:
- Introduced
_compute_nd_scale_shape(...)to derive the correct scale basis for N-D transforms whennormis scaled andsis not provided. - Updated root N-D entry points to use the derived scale basis when computing
fsc. - Added a comprehensive NumPy-reference equivalence test suite covering dtype × layout × axes × norm dispatch paths; documented the fix in the changelog.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
mkl_fft/_fft_utils.py |
Adds _compute_nd_scale_shape to compute the correct normalization basis for scaled norms in N-D transforms (including irfftn output-length handling). |
mkl_fft/_mkl_fft.py |
Switches root N-D FFT wrappers to compute fsc from the transformed-axis scale basis instead of the full array shape. |
mkl_fft/tests/test_dispatch_equivalence.py |
Adds NumPy-reference dispatch/equivalence tests to catch axis/axes/norm scaling and dispatch regressions. |
CHANGELOG.md |
Documents the scaling fixes for subset-axes transforms and irfftn/irfft2 output-length normalization. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Contributor
Author
|
reproducer: import sys
import numpy as np
import mkl_fft
x = np.random.default_rng(0).standard_normal((8, 7, 13)) + 0j
bad = 0
def check(label, got, want):
global bad
f = np.vdot(want, got) / np.vdot(want, want) # least-squares scale
pure = np.allclose(got, f * want) # wrong by ONLY that scale?
ok = abs(f - 1) < 1e-9 and pure
bad += not ok
note = "" if pure else " <-- not a pure scale, values differ too!"
print(f" {'ok ' if ok else 'BUG'} {label:<38} scale={f.real:9.6f}{note}")
print(f"mkl_fft {mkl_fft.__version__}, numpy {np.__version__}, x.shape={x.shape}\n")
print("subset of axes, s not given:")
for axes in [(0,), (1,), (2,), (1, 2)]:
for norm in ("forward", "ortho"):
check(
f"fftn(axes={axes}, norm={norm!r})",
mkl_fft.fftn(x, axes=axes, norm=norm),
np.fft.fftn(x, axes=axes, norm=norm),
)
print("fft2 on a 3-D array -- transforms 2 of 3 axes:")
for norm in ("forward", "ortho"):
check(
f"fft2(norm={norm!r})",
mkl_fft.fft2(x, norm=norm),
np.fft.fft2(x, norm=norm),
)
print("complex-to-real, every axis transformed:")
for fn in ("irfftn", "irfft2"):
for norm in ("forward", "ortho"):
check(
f"{fn}(norm={norm!r})",
getattr(mkl_fft, fn)(x, norm=norm),
getattr(np.fft, fn)(x, norm=norm),
)
print("\ncontrols that should always pass:")
check("fftn(axes=None, norm='ortho')", mkl_fft.fftn(x, norm="ortho"), np.fft.fftn(x, norm="ortho"))
check("fft(axis=1, norm='ortho')", mkl_fft.fft(x, axis=1, norm="ortho"), np.fft.fft(x, axis=1, norm="ortho"))
check("fftn(axes=(0,), norm=None)", mkl_fft.fftn(x, axes=(0,)), np.fft.fftn(x, axes=(0,)))
print(f"\n{bad} mismatched -> bug present" if bad else "\nall match -> fixed")
sys.exit(1 if bad else 0) |
Collaborator
|
@jharlow-intel can you check if this also covers #336? |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Doing some iterative agentic looping dev experimentation. Part of it found this:
Two
normscaling bugs in the root N-D API returned silently mis-scaledresults. Both are fixed by resolving the scale basis from the transformed
axes, matching what
mkl_fft.interfaces.*already does via_cook_nd_args.untransformed axis lengths, because the scale was computed over the full
array shape. Hits
fftn(x, axes=(0,)), and less obviouslyfft2(x)on a3-D array — that transforms 2 of 3 axes.
irfftn/irfft2normalized over the input lengthnrather than thecomplex-to-real output length
2 * (n - 1)along the last transformedaxis. Wrong even when every axis was transformed.
Applies to
fftn/ifftn/rfftn/irfftnand thefft2/ifft2/rfft2/irfft2family withnorm="forward"or"ortho"and no explicits.Unaffected:
norm=None/"backward", explicits=, 1-D transforms, andinterfaces.numpy_fft/scipy_fft.Why it wasn't caught
The existing N-D norm tests compare
mkl_fftagainst othermkl_fftcalls,and
test_fft_with_ordercompares it against itself across memory layouts —self-consistency, never an external reference. The new
test_dispatch_equivalence.pyusesnumpy.fftas the reference acrossdtype × layout × axes × norm, on a shape whose axis lengths all differ so that
an axis permutation cannot produce a correctly shaped result.
Testing
1725 passed / 104 skipped (existing suite was 971 — no regressions).
176 root-API combinations checked against
numpy.fft: 0 mismatches, 32 ofthem failing before the fix. The
norm=Nonepath is unchanged; the new helpershort-circuits in ~0.04 µs.
This is part of other on-going performance improvement looping I'm doing. No rush in merging it, it was entirely agentic and I didn't have time to thoroughly review, hopefully an expert here can say whether the PR is correct or not