Conversation
|
Review these changes at https://app.gitnotebooks.com/stumpy-dev/stumpy/pull/1132 |
There was a problem hiding this comment.
@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 \ |
There was a problem hiding this comment.
It seems that fftw-wisdom is not a MUST have: For instance, see the following Github Acrtions Run:
- https://github.com/NimaSarajpoor/automate/actions/runs/22208636175/job/64241168312
- https://github.com/NimaSarajpoor/automate/actions/runs/22186869860/job/64163022216
I can see thatfftw-wisdom is not available, and pyfftw is installed and tests are passing.
There was a problem hiding this comment.
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.
This is to address
PR 3described in #1118 (comment). Have copied the corresponding notes below: