convolve_fft - counterpart to convolve. NaN-friendly and mask-friendly, N-dimensional convolution - #182

One general comment at first glance: run this through the pep8 tool for whitespace compliance, if you wouldn't mind.

I haven't read this in much detail yet, but two quick notes:

• I think we agreed that it's ok to relax the 80-char limit when it's really awkward to shorten the line - it shouldn't be common, but there are some places where going a few characters over ends up being much more readable than strict compliance.
• I'll take a look at your documentation and see if I can figure out the build problem soon.

I'm not sure there will be any build problem on astropy, but the
formatting looks all wrong when I try it in my local repository.

Which brings me to a question I should perhaps post somewhere else,
but what is the workflow for adding a file to the API documentation?
the docguide gives
some description, but stops short of a complete description... I
assume sphinx-apidoc + sphinx-build both need to be run?

Also, I started running test_convolve's tests with convolve_fft, and
it fails pretty miserably for the 1d tests. I've clearly made some
indexing mistakes that I'm going to clean up before this request
should be accepted.

On Thu, Mar 22, 2012 at 3:31 PM, Erik Tollerud
[email protected]
wrote:

I haven't read this in much detail yet, but two quick notes:

• I think we agreed that it's ok to relax the 80-char limit when it's really awkward to shorten the line - it shouldn't be common, but there are some places where going a few characters over ends up being much more readable than strict compliance.
• I'll take a look at your documentation and see if I can figure out the build problem soon.

---

Reply to this email directly or view it on GitHub:
#182 (comment)

Adam

I'm in the middle of cleaning up the documentation and implementing a standard (mostly automatic) way of doing the API pages. I just pushed a change to master that now makes the documentation for nddata autogenerate appropriately, so if you
add from .convolution.convolve_fft import convolve_fft to the astropy/nddata/__init__.py file, you should get the api documentation to appear automatically on the nddata doc page.

I just tried building the docs after adding from .convolution.convolve_fft import convolve_fft at line 17 of astropy/nddata/__init__.py, and they build fine after the fixes I've described above. (Although note that that's after merging with master, because I just made a change to master that adds the API documentation for nddata).

Here are the latest test results:

https://jenkins.shiningpanda.com/astropy/job/astropy-staging-astrofrog-debian-multiconfig/10/
https://jenkins.shiningpanda.com/astropy/job/astropy-staging-astrofrog-osx-10.7-multiconfig/21/
https://jenkins.shiningpanda.com/astropy/job/astropy-staging-astrofrog-winxp32-multiconfig/14/

On Windows, for some reason, the tests from the unmerged comology branch are being included and that is what is making things fail, but otherwise convolve_fft has the same issue with Numpy 1.4.1 as Mac and Linux.

So precision-wise, it seems all the tests now pass.

I just realized that fftw3 is yet a different package from numpy and scipy (this hadn't hit me before). I guess given the fact there are now three different options for the FFT, it might be better to have an option fft_package= (or something better) with possible options fftw3, scipy, and numpy? (not sure which should be the default). Same goes for the configuration item? Then it will be easier to cycle through each of the three options for the tests, and would be less confusing that having the possibility of setting use_numpy_fft=True, use_scipy_fft=True.

If you just want to merge this in as-is, I'm happy to do some more work on this routine to try and implement my own suggestions.

FWIW, all tests are now passing for me

@astrofrog - I'm not having any problem merging this into master... if I just reset my staging branch to master and merge this one, it does so without complaint...

@keflavich - I agree with @astrofrog that the choice of which FFT to use should be a single option/configuration item that can either be 'scipy','numpy', or 'fftw'. It's very confusing to have three conflicting boolean options... Note that you can specify enum-like configuration options by giving a list of strings as the 'default' value: ConfigurationItem('ffttype', ['fftw','scipy','numpy'],'description...') - whichever one is first in the list ends up as the default. Or alternatively, you could have it be a comma-separated list of strings giving the order in which to try the three options. Either way is fine with me.

Oh, and as for what to do about numpy vs. scipy, I'm fine with either way (merging only the scipy stuff now and working later on numpy, or merging this now and cleaning it up later). Is there any significant performance difference between the numpy and scipy implementations?

@astrofrog I don't think numpy's fft is machine-dependent after all; it was just poorly implemented tests misleading me. So there's no reason not to include it.

@astrofrog, @eteq: I agree completely, ffttype should be an option as you've described. I'll work that in shortly.

I don't know if there's a performance difference between numpy and scipy... I'll change the fft speed tests to find out.

@keflavich - when you implement fft_type, make sure that all three get tested for each test, but also ensure that the tests are skipped if the optional dependencies are not present. See

http://astropy.readthedocs.org/en/latest/development/testguide.html#tests-requiring-optional-dependencies

for information about how to do this. If you have HAS_SCIPY and HAS_FFTW3 then in the tests you could do:

if fft_type == 'fftw3' and not HAS_FFTW3:
    pytest.skip('fftw3 is not installed')
elif fft_type == 'scipy' and not HAS_SCIPY:
    pytest.skip('scipy is not installed')

at the top of each test (I think you have to call pytest.skip rather than using the decorator since you only want to skip some of the parametrized tests).

@astrofrog: Apparently numpy's FFT has some very small performance benefits in some cases. But I'd say my speed tests are rather inconclusive.

I'm going to be picky, but can you use skip rather than not including the options? (which was my first instinct too). We need to know that being skipped due to a missing dependency with the automated testing systems.

And I agree with @astrofrog that it would be better to skip the tests that aren't relevant because of missing dependencies. But if this is going to be very difficult to do, it's also not absolutely necessary.

Also, @astrofrog - I think maybe you're right that one of us should to try to manually merge this after a rebase instead of using the merge button, given the various merge/pull oddities. It may be more trouble than its worth, but whoever does the merging should at least try that first.

@eteq Would it hurt if I tried to rebase from astropy:master at this point? Not sure that it will work, but I may as well try to learn if the exercise is harmless.

@keflavich - well, I would definitely make a copy of the repository and try on that if you want to experiment. I wouldn't call it harmless ;-)

@eteq - I'm happy to seeing to the merging, and I can try a rebase. I can also try and see if I can sort out the skips.

It took a little while, but I managed to rebase. Just one question - I'm getting a one line difference with your code (because I had to resolve merges), and I can't remember if the right version is to have:

if not _ASTROPY_SETUP_:

or:

if not setup_helpers.is_in_build_mode():

which one is the correct one to use? (@eteq?)

Shall I merge? (I'll open a pull request so I can check and merge it via GitHub)

Ok, I opened a pull request (#253) with the rebased convolve_fft code, and have implemented the test skipping and also have made sure that if fft_type is not specified, and FFTW is not installed, it will fallback on scipy, and then on numpy if scipy is not present (I've discussed this off-github with @keflavich).

So shall we close this in favor of #253, then?

@keflavich - #253 (the rebased version of this PR) has been merged, so I'm closing this one. Thanks for your contribution!