perf: skip numpy array creation in _compute_fwd_scale scalar path - #384
Open
intel-python-devops wants to merge 2 commits into
Open
intel-python-devops wants to merge 2 commits into
intel-python-devops wants to merge 2 commits into
Conversation
intel-python-devops
requested review from
antonwolfy,
jharlow-intel,
ndgrigorian,
vlad-perevezentsev and
xaleryb
as code owners
September 28, 2026 14:06
antonwolfy
approved these changes
Sep 28, 2026
antonwolfy
left a comment
Collaborator
There was a problem hiding this comment.
I have a couple of nits, but in overall LGTM.
The PR add few lines of code but reduce memory consumption avoiding temporary np.ndarray allocation.
Comment on lines
+75
to
+80
| # `np.prod` dominates the Python-side cost of a small normalized transform, | ||
| # so take cheaper routes for the two shapes the callers actually pass: a | ||
| # scalar `n` (1-D) and a sequence (`_nd_fwd_scale`). It stays the fallback | ||
| # because `numpy.fft` also accepts array-like `n` and `s` (e.g. a 0-d | ||
| # `n=np.array(8)`, a 1-D `s=np.array([4, 4])`), which `math.prod` cannot | ||
| # handle uniformly. |
Collaborator
There was a problem hiding this comment.
Can we have tighter comment here?
Suggested change
| # `np.prod` dominates the Python-side cost of a small normalized transform, | |
| # so take cheaper routes for the two shapes the callers actually pass: a | |
| # scalar `n` (1-D) and a sequence (`_nd_fwd_scale`). It stays the fallback | |
| # because `numpy.fft` also accepts array-like `n` and `s` (e.g. a 0-d | |
| # `n=np.array(8)`, a 1-D `s=np.array([4, 4])`), which `math.prod` cannot | |
| # handle uniformly. | |
| # Avoid np.prod's 0-d-array overhead on the hot scalar (1-D) and sequence | |
| # (N-D) paths; np.prod stays as the fallback for array-like `n`/`s` | |
| # (e.g. np.array(8)) that math.prod can't handle. |
| # because `numpy.fft` also accepts array-like `n` and `s` (e.g. a 0-d | ||
| # `n=np.array(8)`, a 1-D `s=np.array([4, 4])`), which `math.prod` cannot | ||
| # handle uniformly. | ||
| if isinstance(ss, (int, np.integer)): |
Collaborator
There was a problem hiding this comment.
Could you please populate the changelog?
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.
Note
This pull request is entirely AI-generated. Please review thoroughly.
_compute_fwd_scale runs on every mkl_fft.fft/ifft/rfft/irfft call regardless of norm, and for the scalar 1-D case np.prod(scalar) does nothing but wrap and unwrap a 0-d ndarray, so avoiding it removes pure per-call Python/NumPy overhead on the hottest, smallest-transform code path without changing the computed scale value or its numeric type usage downstream (both python int/float and numpy scalar values convert identically into the Cython
double fscparameter).