Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Simplify replacement of fftpack by pyfftw #5295

Merged
merged 2 commits into from Oct 10, 2015

Conversation

anntzer
Copy link
Contributor

@anntzer anntzer commented Sep 27, 2015

This patch replaces all instances of from scipy.fftpack import ...
by from scipy import fftpack followed by the use of qualified
names (except in the scipy.fftpack submodule itself). The idea is
to simplify the replacement of scipy.fftpack by the FFTW-backed
pyfftw.interfaces.scipy_fftpack, which provides the same interface.

Currently, acceleration of e.g. scipy.signal.fftconvolve
requires fixing individual functions in scipy.signal.signaltools
(scipy.signal.signaltools.fftn = pyfftw.interfaces.scipy_fftpack.fftn; scipy.signal.signaltools.ifftn = pyfftw.interfaces.scipy_fftpack.ifftn)
because signaltools imports the functions out of fftpack.
With the patch, a simple scipy.fftpack.__dict__.update({name: getattr(pyfftw.interfaces.scipy_fftpack, name) for name in pyfftw.interfaces.scipy_fftpack.__all__}) (which could be wrapped in a
helper in pyfftw's side) should suffice.

This patch replaces all instances of `from scipy.fftpack import ...`
by `from scipy import fftpack` followed by the use of qualified
names (except in the `scipy.fftpack` submodule itself).  The idea is
to simplify the replacement of `scipy.fftpack` by the FFTW-backed
`pyfftw.interfaces.scipy_fftpack`, which provides the same interface.

Currently, acceleration of e.g. `scipy.signal.fftconvolve`
requires fixing individual functions in `scipy.signal.signaltools`
(`scipy.signal.signaltools.fftn = pyfftw.interfaces.scipy_fftpack.fftn;
scipy.signal.signaltools.ifftn = pyfftw.interfaces.scipy_fftpack.ifftn`)
because `signaltools` imports the functions out of `fftpack`.
With the patch, a simple `scipy.fftpack.__dict__.update({name:
getattr(pyfftw.interfaces.scipy_fftpack, name) for name in
pyfftw.interfaces.scipy_fftpack.__all__})` (which could be wrapped in a
helper in pyfftw's side) should suffice.
@larsmans larsmans added enhancement A new feature or improvement scipy.signal labels Oct 4, 2015
@rgommers
Copy link
Member

rgommers commented Oct 9, 2015

+1 for a few more characters if it makes using pyfftw easier.

My only question is whether a regression test is needed here. No one will remember this a year from now, so a test running on TravisCI only that inspects source code for lines containing import and fftpack will be quite useful in the long run I expect.

@anntzer
Copy link
Contributor Author

anntzer commented Oct 10, 2015

Added tests (regexp based, excluding doctests; the other possibility would have been to use an AST walker).

@rgommers
Copy link
Member

Learned about something new (pathlib), nice.

One could argue about wanting to run this on other Python versions and/or decorating it with @dec.slow (test takes 3.5s on my machine). But this does the job, so let's call it good.

Thanks @anntzer!

@rgommers rgommers added this to the 0.17.0 milestone Oct 10, 2015
rgommers added a commit that referenced this pull request Oct 10, 2015
MAINT: simplify replacement of fftpack by pyfftw
@rgommers rgommers merged commit 69742ba into scipy:master Oct 10, 2015
rgommers added a commit to rgommers/scipy that referenced this pull request Oct 10, 2015
…ix).

Reason: this small fix interferes with a much larger set of fixes in scipygh-5164
for the same and other ``cluster.dendrogram`` issues.
@anntzer
Copy link
Contributor Author

anntzer commented Oct 10, 2015

This actually takes <0.5s on my machine; perhaps this is due to hard disk access speed?. In fact, I did write a simple AST walker; that took >4s so that's why I came back to the regexp version.

@rgommers
Copy link
Member

Hmm, not sure - I do have a fairly modern machine with a solid state drive. But I'm not too bothered by the extra 3 seconds, so won't look into it now. About once a year I get annoyed and start looking at the largest offenders of test suite runtime - this won't be one of them I think.

@hgomersall
Copy link

Great, thanks for this. I'll update the docs for pyfftw to reflect the changes.

@rgommers
Copy link
Member

We may also want to do the reverse: link to the pyfftw interfaces docs from http://docs.scipy.org/doc/scipy/reference/tutorial/fftpack.html with some explanation. And mention the license differences for background. What do you think?

@hgomersall
Copy link

Good idea. I need to push out the new release once I've fixed a small windows bug which I was planning on sorting it tomorrow. I don't think the documentation link would change though.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement A new feature or improvement scipy.signal
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants