Skip to content

perf: avoid a redundant full-array copy in scipy.fft hfft/hfftn adapters - #382

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

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

Conversation

@intel-python-devops

Copy link
Copy Markdown

Note

This pull request is entirely AI-generated. Please review thoroughly.

perf: avoid a redundant full-array copy in scipy.fft hfft/hfftn adapters

Track: performance

Summary

I read the root AGENTS.md, .github/copilot-instructions.md, and the nested AGENTS.md files, then read _pydfti.pyx, src/mklfft.c.src, _fft_utils.py, _mkl_fft.py, and the interfaces/ adapter modules end to end looking first for descriptor/memory bugs and then for avoidable copies. I checked one suspicious pattern closely — out=x aliasing routed into a DFTI_NOT_INPLACE descriptor — but existing tests (test_fft1d.py test_vector5/6, test_matrix4) already assert that exact case is correct, so I left it alone rather than reopen settled behaviour I can't verify by running. I found and fixed a genuine avoidable copy: mkl_fft.interfaces._scipy_fft.hfft and hfftn each did x = np.array(x, copy=True) followed by np.conjugate(x, out=x), i.e. two full passes over the array, where the sibling _numpy_fft.hfft already uses the equivalent single-pass np.conjugate(x). I replaced both occurrences with x = np.conjugate(x), which allocates the output and negates the imaginary part in one pass instead of two, and verified by re-reading the edited file that this was the only change and that behaviour (result values, dtype, shape, memory layout, and non-mutation of the caller's array) is preserved. I did not touch ihfft/ihfftn (they conjugate a freshly-allocated FFT output in place, so there is no caller-owned buffer at risk there) and did not attempt the higher-ranked but riskier stride-relaxation TODOs in _pydfti.pyx, leaving those to a human who can run the MKL-backed test suite.

Mechanism

hfft/hfftn built the conjugated input via copy-then-mutate: np.array(x, copy=True) (one full memory pass to allocate and copy every element) followed by np.conjugate(x, out=x) (a second full pass to negate every imaginary part in place). Since x is already a plain ndarray by the time this code runs (it went through np.asarray in _validate_input, so it isn't a subclass needing normalization) and the copy's only purpose was to protect the caller's array from the in-place conjugate, np.conjugate(x) without out= does the identical job in one pass: it allocates a new array and writes the conjugated values directly, never touching the input. This halves the memory traffic per element for every call to scipy.fft.hfft/hfftn routed through mkl_fft.interfaces, and is the exact "avoidable copy" pattern ranked second in the mandate's performance priorities.

Why this is safe

The two forms are computed over the same dtypes: _validate_input only rejects float16/float128/complex256, and conjugate is dtype-preserving in both forms (identity-copy for real dtypes, imaginary-negation for complex). Shape is unaffected since conjugate is elementwise. Strides/memory layout are unaffected because both np.array(..., copy=True) and the conjugate ufunc default to order='K' (preserve layout), so the freshly-allocated array in the new code has the same layout characteristics the old two-step form produced. Aliasing/mutation safety is preserved by a different mechanism: the old copy existed so the in-place conjugate wouldn't mutate the caller's x; np.conjugate(x) without out= never writes into x at all, so the caller's array is equally untouched. norm and out parameters are untouched by this diff; hfft/hfftn in this file don't pass out= through to mkl_fft.irfft/irfftn (noted in the existing "overwrite_x is not utilized" comment), so there's no interaction with a caller-supplied output buffer to reason about.

Candidates rejected

  • Relaxing the exact-stride-match / both-contiguous requirement before reusing a caller's out= array in _c2c_fft1d_impl, _r2c_fft1d_impl, _c2r_fft1d_impl (_pydfti.pyx) -- already marked TODO in source; a wrong relaxation risks silently incorrect FFT output and needs the real MKL-backed test suite to validate, which I can't run here.
  • Reordering dtype checks in _downcast_float128_array (interfaces/_float_utils.py) to short-circuit on isinstance(x, np.ndarray) first -- the saving is a couple of cheap dtype-equality comparisons per call; I couldn't state a one-sentence mechanism that clearly separates it from noise, so per the mandate's own test ('if you cannot say why a change is faster in one sentence, it is not a candidate') I left it alone.
  • Treating the out=x aliasing path through a DFTI_NOT_INPLACE descriptor as a correctness bug -- rejected because test_fft1d.py already has three tests (test_vector5, test_vector6, test_matrix4) asserting this exact case is correct and intentional; reopening it without being able to run those tests would be guessing against tested behaviour rather than reasoning from a concrete failure.

Left to a human

  • The stride-relaxation TODOs in _pydfti.pyx for reusing a caller's out= array under a wider set of compatible strides than exact-match / both-contiguous -- higher-ranked opportunity per the mandate but needs MKL documentation review plus a real test run to validate safely.
  • Any compiler/build flag changes -- explicitly out of my scope to implement; I did not identify a specific flag worth proposing this run.
  • CHANGELOG.md wording -- not proposed since this is a performance-only change with no user-visible behaviour difference; the mandate only asks for changelog wording for bug fixes.

@jharlow-intel

Copy link
Copy Markdown
Contributor

seems to be a legitimate performance improvement

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