Repository navigation
Rebased convolve_fft from #182 - #253
Conversation
np.ma.is_masked now used
line that really just got ugly if you shortened it more, and I don't believe in having spaces around a**x - it's one term! well, perhaps I'll do that later...
syntax error. How do you split a for loop's arguments onto two lines? am I going to have to declare the iterators separately?
…n't build right on my machine...
commit to merge, I guess?)
test_convolve_fft (needs to be added) but NOT consistent with convolve! (for good reason - there are some things I don't think should be implemented in fft... or maybe I'm just too lazy to think up how they can be done)
KNOWN BUG: 'boxcar' kernels with even sizes (e.g., [[1,1],[1,1]]) do NOT behave the same in convolve and convolve_fft, and frankly I'm not sure which is right.
default, which just sets everything to zero for the kernel sizes I was testing. __init__ updated to include make_kernel & convolve_fft
(note that the last few commits have violated pep8 in a few minor ways; I think these enhance readability but am open to criticism)
removed fftshift option from convolve_fft - I wasn't convinced it did anything useful, and it wasn't clear what I wanted it to do. We can add that feature back after writing some unit tests if it is actually desirable
**'s because "array" is a parameter, but backticks cause intersphinx to point at python's "array" builtin. Workarounds?
USE_NUMPY_FFT added as a configurationoption
different treatment of numpy & fftw. fftw is apparently more precise than numpy, but my guess is that ymmv depending on machine/install
overridden with kwargs) and testing using different FFT types.
behaved at higher precision than numpy's fft. Removed np 1.4 incompatible float16 usage
Addresses @astrofrog's comment from 20 seconds ago...
loop if has_fftw == False)
functional py.test
…ot available. Also ensure that convolve_fft does not crash if fft_type=None and the dependency needed for the default is not available (if fftw3 is not available, a warning is raised to say that scipy will be used, and if scipy is not available, a warning is raised to say that numpy will be used).
|
I need to fix a failing test with Numpy 1.4.1 before I can merge this. |
There was a problem hiding this comment.
I see that this is done here, but then the final cast in the return statement is never done (although it is in convolve) - is this intentional? Right now nothing seems to use array_dtype...
There was a problem hiding this comment.
Pyflakes told me that too, but it is used in the return statement. Maybe a pyflakes bug?
There was a problem hiding this comment.
It isn't used in the return statement for convolve_fft, only convolve. Shouldn't .astype(array_dtype) be added to all of the return statements in convolve_fft? (I count 5 of them...)
There was a problem hiding this comment.
Ah right, I'll take a look at that.
There was a problem hiding this comment.
You are right that it's defined but not used in convolve_fft. If we do implement that like in convolve, we need some tests for it, so I'm just going to merge this as-is, and will work on a PR that implements this.
|
All the tests pass for me, so aside from that one comment and whatever you need to fix for numpy 1.4.1, as far as I'm concerned this is good to merge. |
…atible with Numpy 1.4.1
Rebased convolve_fft from #182
Rebased convolve_fft from astropy#182
Replace Tom with Thomas
This is a rebased version of #182 to ensure that it is fine before merging.