Skip to content

Comments

Add pyfftw sdp#1132

Open
NimaSarajpoor wants to merge 10 commits intostumpy-dev:mainfrom
NimaSarajpoor:add_pyfftw_sdp
Open

Add pyfftw sdp#1132
NimaSarajpoor wants to merge 10 commits intostumpy-dev:mainfrom
NimaSarajpoor:add_pyfftw_sdp

Conversation

@NimaSarajpoor
Copy link
Collaborator

This is to address PR 3 described in #1118 (comment). Have copied the corresponding notes below:

  1. Add _PYFFTW_SLIDING_DOT_PRODUCT to sdp.py
  2. Add unit tests that demonstrate that _PYFFTW_SLIDING_DOT_PRODUCT matches the results of naive_rolling_window_dot_product
  3. Handle the test.sh check_fftw_pyfftw stuff here too

@gitnotebooks
Copy link

gitnotebooks bot commented Feb 17, 2026

Review these changes at https://app.gitnotebooks.com/stumpy-dev/stumpy/pull/1132

Copy link
Contributor

@seanlaw seanlaw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@NimaSarajpoor I've left some comments for you to address. I think we can afford to be clearer since all of this pyfftw stuff will be hard to maintain. We should probably be as verbose (and add more comments cross referencing their docs as possible). I'll do another few passes after you've responded and made modifications. I think this pyfftw stuff needs to be crystal clear

{
# If `ray` command is not found then generate a .coveragerc_override file
if ! command -v ray &> /dev/null
if ! command -v fftw-wisdom &> /dev/null \
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems that fftw-wisdom is not a MUST have: For instance, see the following Github Acrtions Run:

I can see thatfftw-wisdom is not available, and pyfftw is installed and tests are passing.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, what should we do?

Copy link
Collaborator Author

@NimaSarajpoor NimaSarajpoor Feb 23, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should remove the check command -v fftw-wisdom from ./test.sh.

There is still something that I cannot figure out though. If pyfftw can be imported with no error, does that mean it can compute fft for any dtype listed in the Schema table in pyfftw doc? I think the answer is "No". Because I noticed the issue pyFFTW/pyFFTW#70 says:

In some Linux distributions it is common to have only the fftw3 library and not fftw3f. PyFFTW should detect that it is not available and work anyways (just not accept float32/single-precision inputs).

In addition, what I infer from these lines of code in pyfftw is that pyfftw might be installed successfully based on some available fftw* but it may not work for all supported dtypes. In our code, dtype is set to np.float64. One way to check is to use wisdom returned by pyfftw.export_wisdom():

Return the FFTW wisdom as a tuple of strings.
The first string in the tuple is the string for the double precision wisdom, the second is for single precision, and the third for long double precision. If any of the precisions is not supported in the build, the string is empty.

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