Skip to content

fix: cast unsupported-dtype input to complex128 when out= is given - #387

Open
intel-python-devops wants to merge 3 commits into
masterfrom
chore/agentic-sweep-13
Open

intel-python-devops wants to merge 3 commits into
masterfrom
chore/agentic-sweep-13

Conversation

@intel-python-devops

@intel-python-devops intel-python-devops commented Oct 5, 2026 •

Copy link
Copy Markdown

_c2c_fft1d_impl returned out unmodified when out= was given and the input dtype was not float32/float64/complex64/complex128 (e.g. integer, bool, float16). The cast to complex128 was skipped whenever out was passed, so nothing was dispatched to MKL and the zero status was reported as success.

Affected entry points:

  • mkl_fft.fft / ifft
  • mkl_fft.fftn / ifftn / fft2 / ifft2 (via _iter_fftnd)
  • mkl_fft.interfaces.numpy_fft and patched numpy.fft

mkl_fft.interfaces.scipy_fft is not affected.

Changes in mkl_fft/_pydfti.pyx:

  • The cast now always happens; out is not None forces out-of-place afterwards, as in _direct_fftnd.
  • Whether MKL may write directly into out is now decided by comparing out's strides with x_arr (the cast or padded copy that MKL reads) instead of the original x. Comparing against x gave wrong results and writes outside out for rank >= 3 input when x and out shared a non-contiguous or F-ordered layout. This affected the newly enabled cast path and, already on master, complex input with n= zero-padding.
  • If an unsupported type reaches the out-of-place dispatch, it now raises ValueError instead of returning silently.

Behaviour change: on platforms where long double is wider than double, longdouble/clongdouble input with out= now raises ValueError, as it already did without out, instead of returning out unmodified.

Tests (in test_fft1d.py and test_fftnd.py):

  • bool, int8, uint8, int32, int64 and float16 with out=, including n truncation and padding
  • rank-3 strided and F-ordered out, checking that nothing outside out is written
  • longdouble raising ValueError
  • fftn / ifftn with integer input

CHANGELOG entries added under [dev] / Fixed.

`_c2c_fft1d_impl` decided whether `out` can be written directly by MKL by
comparing its strides with the original `x`, but MKL reads `x_arr`, which
may be a cast or zero-padded contiguous copy of `x`. When `x` and `out`
shared a non-contiguous or F-ordered layout, rank >= 3 transforms gave
wrong results and wrote outside of `out`. With the cast now also done
when `out` is given, this affected integer/bool/float16 input as well as
the pre-existing case of padded complex input.

Also restore the original dtype-dispatch structure and apply the `out`
override after the cast, as `_direct_fftnd` does; raise instead of
silently returning when the out-of-place dispatch sees an unsupported
type; move the regression tests into test_fft1d.py/test_fftnd.py with
coverage for strided, F-ordered, truncated and padded layouts and for
fftn/ifftn; and add CHANGELOG entries.
@antonwolfy

Copy link
Copy Markdown
Collaborator

Pushed 2417932 on top of the original fix.

Fixes

  • The stride check now uses the array MKL actually reads. _c2c_fft1d_impl decided whether MKL can write straight into out by comparing out's strides with the original x. But MKL reads x_arr, which may be a cast or zero-padded contiguous copy of x. Now that this PR also casts when out= is given, rank ≥ 3 int/bool/float16 input whose out has the same non-contiguous or F-ordered layout gave wrong values, and wrote outside out (it segfaults under a guard page):
    base = np.full((4, 6, 8), -1 - 1j); out = base[:, :3, :]
    x = np.arange(192).reshape(4, 6, 8)[:, :3, :]
    mkl_fft.fft(x, out=out)  # wrong values; 48 elements of `base` outside `out` overwritten
    Complex input with n= zero-padding had the same bug on master; that's fixed as well.
  • If an unsupported type reaches the out-of-place dispatch, it now raises ValueError instead of returning out unchanged.

Cleanup

  • The dtype branches are back to their original structure. if out is not None: in_place = 0 now runs after the cast, as in _direct_fftnd, and the long comment is down to one line.
  • The regression tests moved into test_fft1d.py and test_fftnd.py, which also fixes the flake8 D209 pre-commit failure. They now cover:
    • bool, int8, uint8, int32, int64 and float16, with n truncation and padding;
    • rank-3 strided and F-ordered out, checking that nothing outside out is written;
    • longdouble raising ValueError;
    • fftn/ifftn with integer input.
  • Added CHANGELOG entries.

Results for the 74 new tests: 58 fail on master and 18 fail on the previous PR head (all of them strided or F-ordered cases). On this commit they all pass, as does the full suite.

@antonwolfy antonwolfy added this to the 2.4.0 release milestone Oct 6, 2026
@antonwolfy

This comment was marked as resolved.

@antonwolfy antonwolfy self-assigned this Oct 6, 2026
@antonwolfy antonwolfy changed the title fix: cast unsupported-dtype input to complex128 even when out= is give fix: cast unsupported-dtype input to complex128 when out= is given Oct 6, 2026
@antonwolfy

Copy link
Copy Markdown
Collaborator

Performance results for this PR (head 70166a9) against master (e06bd74).

Setup. Intel Core Ultra 5 236V (Lunar Lake: 4 P-cores and 4 LP E-cores), Ubuntu 25.10, Python 3.13, NumPy 2.5.3, oneMKL 2026.2. Both builds were compiled from source with gcc 15.2 and pass the full test suite (2625 passed on master, 2699 here, which adds the new tests).

  • The CPU governor was set to performance.
  • Runs were pinned to P-cores: 1 thread on a single P-core, and 4 threads on all four.
  • In the final run, each build was measured in 10 fresh processes, in shuffled order. As a noise control, master was also run as a byte-identical copy of itself (A/A). Figures are medians over processes.

Summary. The paths that already worked are unchanged. out= with zero-padded input along the last axis is up to 2x faster. The newly working cast path (integer, bool, float16 input with out=) performs about as well as the existing work-arounds.

1. Zero-padded input with out= (n larger than the input): faster

On master the strides of the original input never matched out here, so it computed into a temporary and copied. Comparing against x_arr (the padded copy that MKL reads) lets it write into out directly.

fft(x, n=..., out=out) master this PR speedup, 1 thread master this PR speedup, 4 threads
256×1000 complex128, n=1024, last axis 905 µs 572 µs 1.58x 718 µs 363 µs 1.98x
256×1000 float64, n=1024, last axis 741 µs 518 µs 1.43x 593 µs 334 µs 1.78x
64×64×50 complex128, n=64, last axis 719 µs 492 µs 1.46x 674 µs 361 µs 1.86x
1000×256 complex128, n=1024, axis 0 1548 µs 1553 µs 1.00x 786 µs 777 µs 1.01x

The axis-0 case is unchanged because the strides of x and out already matched on master. The speedup does not depend on array alignment.

2. Paths that already worked: no change

These are 23 cases: 1-D fft/ifft for complex128, complex64 and float32 input (n = 1024 to 2^20), with contiguous out and without out; 3-D 64³ on axes 0 and 2; complex input with a strided out of the same layout; and fft(x) / o[...] = fft(x) for int32, bool and float16 input.

geometric mean of the per-case ratios 4 threads 1 thread
this PR / master 0.982 1.001
master copy / master (A/A) 0.982 1.001

The PR is exactly as far from master as an identical copy of master is. Comparing only processes whose arrays landed on the same 64-byte alignment, this PR against master gives 0.995 (4 threads) and 1.001 (1 thread).

3. Cast path with out=: now works, cost in line with the work-arounds

On master fft(x, out=o) returned o unmodified for these dtypes, so the comparison is against what users do instead. Medians for int64 input at 1 thread / 4 threads (int32, bool and float16 behave the same within noise):

int64 input fft(x, out=o) (this PR) master: o[...] = fft(x) master: fft(x) np.fft.fft(x, out=o)
65536 0.23 / 0.14 ms 0.23 / 0.25 ms 0.20 / 0.15 ms 0.60 / 1.00 ms
2^20 8.8 / 5.0 ms 8.4 / 5.8 ms 7.9 / 5.0 ms 23.4 / 20.1 ms
64³ 0.43 / 0.28 ms 0.51 / 0.57 ms 0.31 / 0.25 ms 0.79 / 0.89 ms
  • Compared with o[...] = fft(x): at 1 thread it is within about ±15% (4% slower at 2^20, 16% faster at 64³); at 4 threads it is 15–50% faster, about 2x for the 64³ array.
  • fft(x) without out is somewhat faster because it transforms the converted copy in place, but it has no out.
  • It is about 2–7x faster than np.fft.fft(x, out=o).
  • Peak memory is the same as the work-arounds.
  • With a strided out (a 64³ view), the result goes through a temporary and is copied, so that case takes about 4–11% longer than view[...] = fft(x) and peaks at 8 MiB instead of 4 MiB. Before the stride fix in 2417932, this case wrote wrong values and wrote outside out.
  • These cast-path timings are medians over 3 rounds. They are not controlled for alignment and carry about ±15% noise.

A note on measurement noise

In a first 4-thread run, some unchanged cases looked 20–70% slower with this PR, for example fft of n=16384 complex64 at 25 µs versus 14 µs. That turned out to be array alignment, not the code: at the same alignment both builds take about 24 µs at 16/48 bytes and 14–15 µs at 0/32 bytes, and the identical master copy reproduced the same gaps. So single-case swings of ±30% in ASV or Jenkins benchmark results for this PR should not be read as regressions without checking alignment.

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.

2 participants