From 8dc0bf640faabb3e9cb7510286aade73da01f5b3 Mon Sep 17 00:00:00 2001 From: Python Infra CI Date: Mon, 21 Sep 2026 15:19:20 +0000 Subject: [PATCH 1/2] perf: avoid a redundant full-array copy in scipy.fft hfft/hfftn adapters --- mkl_fft/interfaces/_scipy_fft.py | 12 ++- pr-body.md | 168 +++++++++++++++++++++++++++++++ pr-title.txt | 1 + 3 files changed, 177 insertions(+), 4 deletions(-) create mode 100644 pr-body.md create mode 100644 pr-title.txt diff --git a/mkl_fft/interfaces/_scipy_fft.py b/mkl_fft/interfaces/_scipy_fft.py index 89383233..f9e84042 100644 --- a/mkl_fft/interfaces/_scipy_fft.py +++ b/mkl_fft/interfaces/_scipy_fft.py @@ -523,8 +523,10 @@ def hfft( _check_plan(plan) x = _validate_input(x) norm = _swap_direction(norm) - x = np.array(x, copy=True) - np.conjugate(x, out=x) + # np.conjugate(x) already allocates a fresh array, so there is no need + # to copy x first and then conjugate it in place (which would touch + # every element twice instead of once). + x = np.conjugate(x) with _Workers(workers): # Note: overwrite_x is not utilized @@ -626,8 +628,10 @@ def hfftn( _check_plan(plan) x = _validate_input(x) norm = _swap_direction(norm) - x = np.array(x, copy=True) - np.conjugate(x, out=x) + # np.conjugate(x) already allocates a fresh array, so there is no need + # to copy x first and then conjugate it in place (which would touch + # every element twice instead of once). + x = np.conjugate(x) s, axes = _init_nd_shape_and_axes(x, s, axes, invreal=True) with _Workers(workers): diff --git a/pr-body.md b/pr-body.md new file mode 100644 index 00000000..02073592 --- /dev/null +++ b/pr-body.md @@ -0,0 +1,168 @@ +## Track + +Performance (Track A), item 2 on the ranked list: an avoidable copy in +`_fft_utils.py` / interface layer — here `mkl_fft/interfaces/_scipy_fft.py`. + +## What I examined + +I read the root `AGENTS.md`, `.github/copilot-instructions.md`, and the nested +`AGENTS.md` files for `mkl_fft/`, `mkl_fft/interfaces/`, `mkl_fft/src/`, and +`mkl_fft/tests/` before touching anything. I then read `_pydfti.pyx`, +`_fft_utils.py`, `_mkl_fft.py`, `src/mklfft.c.src`, and the three interface +adapter files (`_numpy_fft.py`, `_scipy_fft.py`, `_float_utils.py`, +`_numpy_helper.py`) end to end, looking first for descriptor-handling or +memory-management bugs per the bug-fix priority list, then for avoidable +copies and per-call overhead per the performance priority list. + +I specifically checked the `out=x` aliasing path in `_c2c_fft1d_impl` +(`_pydfti.pyx`) against the MKL descriptor being configured +`DFTI_PLACEMENT = DFTI_NOT_INPLACE` in the `*_out` C routines +(`src/mklfft.c.src`), since passing identical input/output pointers to a +not-in-place descriptor looked suspicious. It turned out `mkl_fft/tests/ +test_fft1d.py` (`test_vector5`, `test_vector6`, `test_matrix4`) already +exercises exactly this case as a documented, presumably-passing feature +("fft in-place is the same as fft out-of-place"), so I did not treat this as +a new finding and left it alone — reopening settled, tested behaviour without +being able to run the suite myself would be speculative. + +## What I found + +In `mkl_fft/interfaces/_scipy_fft.py`, `hfft` and `hfftn` each computed the +Hermitian-conjugate of their input as two full passes over the array: + +```python +x = np.array(x, copy=True) # pass 1: copy every element +np.conjugate(x, out=x) # pass 2: negate every imaginary part in place +``` + +`x` at this point has already been through `_validate_input`, which calls +`np.asarray(x)` (via `_supported_array_or_not_implemented`), so it is always +a plain `ndarray` (subclasses are already stripped, `subok=False` is +`np.asarray`'s default) — the explicit copy exists solely so that the +in-place `conjugate` does not mutate the caller's own array, not for any +type normalization. `np.conjugate(x)` called without `out=` already +allocates a fresh output array and computes the conjugate directly from the +input in a single pass, giving the identical result without the caller's +array ever being touched. The two-step form is therefore a copy taken +defensively where a single elementwise op already does the job — exactly the +"avoidable copies" pattern called out as the second-ranked optimization +target in the mandate. The sibling implementation `_numpy_fft.hfft` already +uses the single-call form (`np.conjugate(x)`), so this is also a small +internal inconsistency between the two interface adapters. + +## The change + +In both `hfft` and `hfftn` in `mkl_fft/interfaces/_scipy_fft.py`, replaced: + +```python +x = np.array(x, copy=True) +np.conjugate(x, out=x) +``` + +with: + +```python +x = np.conjugate(x) +``` + +`ihfft`/`ihfftn` were not touched: they conjugate the *output* of `rfft`/ +`rfftn` in place, and that output buffer was just freshly allocated by +`mkl_fft.rfft`/`rfftn` for this call, so there is no caller-owned buffer at +risk and no redundant copy to remove — the one-pass in-place conjugate there +is already optimal. + +## Why this is behaviour-preserving + +- **Correctness of the result:** `np.conjugate(a)` and + `(lambda b: (np.conjugate(b, out=b), b)[1])(np.array(a, copy=True))` + compute the same values element-by-element; the only difference is whether + the negation of the imaginary part happens into a copy-then-mutate buffer + or straight into a freshly allocated one. +- **dtype:** unaffected — conjugate preserves the input dtype in both forms + (real dtypes are a no-op copy, complex dtypes negate the imaginary part); + `x` here is always a plain array of a dtype `_validate_input` already + approved (not `float16`/`float128`/`complex256`). +- **Shape:** unaffected — `conjugate` is elementwise and shape-preserving in + both forms. +- **Strides / memory layout:** both `np.array(x, copy=True)` and the + `conjugate` ufunc default to `order='K'` (preserve layout as far as + possible), so the freshly allocated array has the same layout + characteristics the old two-step form produced. +- **Aliasing:** the old code copied specifically so the in-place conjugate + would not mutate the caller's array. `np.conjugate(x)` without `out=` + never writes into `x`, so the caller's array is equally untouched under + the new code — the safety property the copy existed for is preserved by a + different, cheaper mechanism. +- **norm/out parameters:** untouched by this diff; `hfft`/`hfftn` do not + accept an `out=` argument to `mkl_fft.irfft`/`irfftn` in this file (`# Note: + overwrite_x is not utilized`), so there is no interaction with an + output buffer supplied by the caller. + +## Mechanism, one sentence + +Two full sequential passes over the array (allocate-and-copy, then negate +imaginary parts in place) are replaced by one pass that allocates and +negates at the same time, halving the memory traffic per element for every +call to `scipy.fft.hfft`/`hfftn` routed through `mkl_fft.interfaces`. + +## Verification + +I have no shell access in this environment, so I could not run `pytest`, +`black`, `isort`, `flake8`, `pylint`, or `cython-lint` myself. I re-read the +edited file in full after the change (shown above) to confirm the two edits +are the only difference from the original and that indentation/imports are +unchanged; `numpy` was already imported at module scope, so no import +changes were needed. I did not add a new test because this is a +performance-only change with no behavioural difference to assert beyond what +the existing `scipy.fft` compatibility tests already cover (`mkl_fft/tests/ +third_party/scipy/test_basic.py` exercises `hfft`/`hfftn` through +`mkl_fft.interfaces.scipy_fft`); CI's existing suite is the check that the +result is still numerically identical, and CI's ASV benchmark is the +authority on whether it is faster. + +Commands run: none (no shell in this environment). Local measurement: none +taken; this report states the mechanism (one memory pass instead of two) as +the basis for the hypothesis, per the mandate, and leaves confirmation to +CI's benchmark job. + +## Rejected candidates + +- Relaxing the exact-stride-match requirement before reusing a caller's + `out=` array in `_c2c_fft1d_impl` (`_pydfti.pyx`), which is marked `TODO` + in the source. This would remove a real defensive copy for a wider range + of strided `out=` arrays, but the comment already flags it as needing + careful validation of what MKL actually tolerates for input/output stride + relationships, and getting it wrong risks silently wrong FFT output. Left + as a candidate for a human with the ability to run the MKL-backed test + suite. +- Reordering the dtype checks in `_downcast_float128_array` + (`interfaces/_float_utils.py`) to test `isinstance(x, np.ndarray)` before + the `longdouble`/`clongdouble` comparisons, to skip redundant work for the + common plain-`ndarray` case. The saving is a couple of dtype equality + checks per call — real but small enough that I could not state a clear + single-sentence mechanism distinguishing it from noise, so I left it alone + per "if you cannot say why a change is faster in one sentence, it is not a + candidate." +- Treating the `out=x` (aliasing) path through a `DFTI_NOT_INPLACE` + descriptor in `_pydfti.pyx`/`src/mklfft.c.src` as a bug. Rejected because + existing tests (`test_fft1d.py::Test_mklfft_vector::test_vector5/6`, + `Test_mklfft_matrix::test_matrix4`) already assert this exact case is + correct and treat it as a supported feature; reopening it without being + able to run those tests would be guessing against settled, tested + behaviour. + +## Left to humans + +- The stride-relaxation `TODO`s in `_pydfti.pyx` (`_c2c_fft1d_impl`, + `_r2c_fft1d_impl`, `_c2r_fft1d_impl`) for reusing a caller's `out=` array + under a wider set of compatible strides than exact match / both-contiguous. + This is the higher-ranked "descriptor handling" and "avoidable copies" + opportunity in the mandate, but validating exactly what MKL DFTI accepts + for mismatched input/output strides needs to be checked against MKL + documentation and run against the real test suite, which I cannot do here. +- Any compiler/build flag changes (explicitly out of scope for me to + implement per the mandate; I did not find a specific one to propose this + run). +- `CHANGELOG.md` wording: not proposed, because this run made no + user-visible behaviour change (performance-only), and the mandate only + asks for changelog wording for bug fixes. diff --git a/pr-title.txt b/pr-title.txt new file mode 100644 index 00000000..681cc780 --- /dev/null +++ b/pr-title.txt @@ -0,0 +1 @@ +perf: avoid a redundant full-array copy in scipy.fft hfft/hfftn adapters From 6c1f37a3d1d29cd6376750305712e404599b1ee7 Mon Sep 17 00:00:00 2001 From: "Harlow, Jordan" Date: Mon, 21 Sep 2026 10:04:57 -0600 Subject: [PATCH 2/2] fix: remove generated doc files --- pr-body.md | 168 --------------------------------------------------- pr-title.txt | 1 - 2 files changed, 169 deletions(-) delete mode 100644 pr-body.md delete mode 100644 pr-title.txt diff --git a/pr-body.md b/pr-body.md deleted file mode 100644 index 02073592..00000000 --- a/pr-body.md +++ /dev/null @@ -1,168 +0,0 @@ -## Track - -Performance (Track A), item 2 on the ranked list: an avoidable copy in -`_fft_utils.py` / interface layer — here `mkl_fft/interfaces/_scipy_fft.py`. - -## What I examined - -I read the root `AGENTS.md`, `.github/copilot-instructions.md`, and the nested -`AGENTS.md` files for `mkl_fft/`, `mkl_fft/interfaces/`, `mkl_fft/src/`, and -`mkl_fft/tests/` before touching anything. I then read `_pydfti.pyx`, -`_fft_utils.py`, `_mkl_fft.py`, `src/mklfft.c.src`, and the three interface -adapter files (`_numpy_fft.py`, `_scipy_fft.py`, `_float_utils.py`, -`_numpy_helper.py`) end to end, looking first for descriptor-handling or -memory-management bugs per the bug-fix priority list, then for avoidable -copies and per-call overhead per the performance priority list. - -I specifically checked the `out=x` aliasing path in `_c2c_fft1d_impl` -(`_pydfti.pyx`) against the MKL descriptor being configured -`DFTI_PLACEMENT = DFTI_NOT_INPLACE` in the `*_out` C routines -(`src/mklfft.c.src`), since passing identical input/output pointers to a -not-in-place descriptor looked suspicious. It turned out `mkl_fft/tests/ -test_fft1d.py` (`test_vector5`, `test_vector6`, `test_matrix4`) already -exercises exactly this case as a documented, presumably-passing feature -("fft in-place is the same as fft out-of-place"), so I did not treat this as -a new finding and left it alone — reopening settled, tested behaviour without -being able to run the suite myself would be speculative. - -## What I found - -In `mkl_fft/interfaces/_scipy_fft.py`, `hfft` and `hfftn` each computed the -Hermitian-conjugate of their input as two full passes over the array: - -```python -x = np.array(x, copy=True) # pass 1: copy every element -np.conjugate(x, out=x) # pass 2: negate every imaginary part in place -``` - -`x` at this point has already been through `_validate_input`, which calls -`np.asarray(x)` (via `_supported_array_or_not_implemented`), so it is always -a plain `ndarray` (subclasses are already stripped, `subok=False` is -`np.asarray`'s default) — the explicit copy exists solely so that the -in-place `conjugate` does not mutate the caller's own array, not for any -type normalization. `np.conjugate(x)` called without `out=` already -allocates a fresh output array and computes the conjugate directly from the -input in a single pass, giving the identical result without the caller's -array ever being touched. The two-step form is therefore a copy taken -defensively where a single elementwise op already does the job — exactly the -"avoidable copies" pattern called out as the second-ranked optimization -target in the mandate. The sibling implementation `_numpy_fft.hfft` already -uses the single-call form (`np.conjugate(x)`), so this is also a small -internal inconsistency between the two interface adapters. - -## The change - -In both `hfft` and `hfftn` in `mkl_fft/interfaces/_scipy_fft.py`, replaced: - -```python -x = np.array(x, copy=True) -np.conjugate(x, out=x) -``` - -with: - -```python -x = np.conjugate(x) -``` - -`ihfft`/`ihfftn` were not touched: they conjugate the *output* of `rfft`/ -`rfftn` in place, and that output buffer was just freshly allocated by -`mkl_fft.rfft`/`rfftn` for this call, so there is no caller-owned buffer at -risk and no redundant copy to remove — the one-pass in-place conjugate there -is already optimal. - -## Why this is behaviour-preserving - -- **Correctness of the result:** `np.conjugate(a)` and - `(lambda b: (np.conjugate(b, out=b), b)[1])(np.array(a, copy=True))` - compute the same values element-by-element; the only difference is whether - the negation of the imaginary part happens into a copy-then-mutate buffer - or straight into a freshly allocated one. -- **dtype:** unaffected — conjugate preserves the input dtype in both forms - (real dtypes are a no-op copy, complex dtypes negate the imaginary part); - `x` here is always a plain array of a dtype `_validate_input` already - approved (not `float16`/`float128`/`complex256`). -- **Shape:** unaffected — `conjugate` is elementwise and shape-preserving in - both forms. -- **Strides / memory layout:** both `np.array(x, copy=True)` and the - `conjugate` ufunc default to `order='K'` (preserve layout as far as - possible), so the freshly allocated array has the same layout - characteristics the old two-step form produced. -- **Aliasing:** the old code copied specifically so the in-place conjugate - would not mutate the caller's array. `np.conjugate(x)` without `out=` - never writes into `x`, so the caller's array is equally untouched under - the new code — the safety property the copy existed for is preserved by a - different, cheaper mechanism. -- **norm/out parameters:** untouched by this diff; `hfft`/`hfftn` do not - accept an `out=` argument to `mkl_fft.irfft`/`irfftn` in this file (`# Note: - overwrite_x is not utilized`), so there is no interaction with an - output buffer supplied by the caller. - -## Mechanism, one sentence - -Two full sequential passes over the array (allocate-and-copy, then negate -imaginary parts in place) are replaced by one pass that allocates and -negates at the same time, halving the memory traffic per element for every -call to `scipy.fft.hfft`/`hfftn` routed through `mkl_fft.interfaces`. - -## Verification - -I have no shell access in this environment, so I could not run `pytest`, -`black`, `isort`, `flake8`, `pylint`, or `cython-lint` myself. I re-read the -edited file in full after the change (shown above) to confirm the two edits -are the only difference from the original and that indentation/imports are -unchanged; `numpy` was already imported at module scope, so no import -changes were needed. I did not add a new test because this is a -performance-only change with no behavioural difference to assert beyond what -the existing `scipy.fft` compatibility tests already cover (`mkl_fft/tests/ -third_party/scipy/test_basic.py` exercises `hfft`/`hfftn` through -`mkl_fft.interfaces.scipy_fft`); CI's existing suite is the check that the -result is still numerically identical, and CI's ASV benchmark is the -authority on whether it is faster. - -Commands run: none (no shell in this environment). Local measurement: none -taken; this report states the mechanism (one memory pass instead of two) as -the basis for the hypothesis, per the mandate, and leaves confirmation to -CI's benchmark job. - -## Rejected candidates - -- Relaxing the exact-stride-match requirement before reusing a caller's - `out=` array in `_c2c_fft1d_impl` (`_pydfti.pyx`), which is marked `TODO` - in the source. This would remove a real defensive copy for a wider range - of strided `out=` arrays, but the comment already flags it as needing - careful validation of what MKL actually tolerates for input/output stride - relationships, and getting it wrong risks silently wrong FFT output. Left - as a candidate for a human with the ability to run the MKL-backed test - suite. -- Reordering the dtype checks in `_downcast_float128_array` - (`interfaces/_float_utils.py`) to test `isinstance(x, np.ndarray)` before - the `longdouble`/`clongdouble` comparisons, to skip redundant work for the - common plain-`ndarray` case. The saving is a couple of dtype equality - checks per call — real but small enough that I could not state a clear - single-sentence mechanism distinguishing it from noise, so I left it alone - per "if you cannot say why a change is faster in one sentence, it is not a - candidate." -- Treating the `out=x` (aliasing) path through a `DFTI_NOT_INPLACE` - descriptor in `_pydfti.pyx`/`src/mklfft.c.src` as a bug. Rejected because - existing tests (`test_fft1d.py::Test_mklfft_vector::test_vector5/6`, - `Test_mklfft_matrix::test_matrix4`) already assert this exact case is - correct and treat it as a supported feature; reopening it without being - able to run those tests would be guessing against settled, tested - behaviour. - -## Left to humans - -- The stride-relaxation `TODO`s in `_pydfti.pyx` (`_c2c_fft1d_impl`, - `_r2c_fft1d_impl`, `_c2r_fft1d_impl`) for reusing a caller's `out=` array - under a wider set of compatible strides than exact match / both-contiguous. - This is the higher-ranked "descriptor handling" and "avoidable copies" - opportunity in the mandate, but validating exactly what MKL DFTI accepts - for mismatched input/output strides needs to be checked against MKL - documentation and run against the real test suite, which I cannot do here. -- Any compiler/build flag changes (explicitly out of scope for me to - implement per the mandate; I did not find a specific one to propose this - run). -- `CHANGELOG.md` wording: not proposed, because this run made no - user-visible behaviour change (performance-only), and the mandate only - asks for changelog wording for bug fixes. diff --git a/pr-title.txt b/pr-title.txt deleted file mode 100644 index 681cc780..00000000 --- a/pr-title.txt +++ /dev/null @@ -1 +0,0 @@ -perf: avoid a redundant full-array copy in scipy.fft hfft/hfftn adapters