GalSim-developers/GalSim — pull request #440 by rmjarvis (2013-06-17 06:19:00+00:00)
Title: #432 Some bug fixes for InterpolatedImage

This has essentially two different bug fixes for InterpolatedImages.

- The first was done in #389. The implementation of our `pad_image` parameter had a few problems associated with it. Most notably, if the `pad_image` was a "good FFT size" already, then the code in SBInterpolatedImage actually wrote to (and read from) one element past the allocated memory, which led to either segmentation faults or wrong answers.
- When investigating this problem, I discovered that there weren't any unit tests that dealt with the `pad_image` parameter. So I wrote some up, and along the way discovered a couple other more minor bugs.
- Related to this, Melanie and I also removed a lot of the login in the RealGalaxy constructor that was repeated in the InterpolatedImage constructor. Now the code for dealing with `pad_image`, `pad_factor`, etc. only appears in the InterpolatedImage constructor, so the RealGalaxy constructor is much simpler.
- The other bug was reported in #432 by Rachel and Matt. The functions in SBInterpolatedImage that are used by the real-space convolve routines were buggy. Actually, worse -- they were completely wrong and used a variable that is never initialized. Anyway, I fixed them up so they work correctly now.
- Yet again, there was no unit test for this. While I think it is almost always a bad idea to use real-space convolution for InterpolatedImages, we do need to be sure it is working right (and remains so going forward). So I added tests that the real-space convolution matches the FFT convolution for a number of interpolants for 15x15 images. I don't recommend going much larger than this in general, since the running time for larger images kind of blows up, especially for Lanczos interpolation.
- Along the way, I noticed that the table lookup in Lanczos xval is slower than the direct calculation with sin(pi x) * sin(pi x/n). Even better, for small integer n values, you can expand out the trig to make it so only one sin call is required.
- The aforementioned unit tests for Lanczos interpolants are disabled. They pass the tests just fine, but the running time is too long for a unit test, even with the small images being tested and even with the above speed-up. Developers working on this should re-enable the tests during development testing. But unless someone speeds up the code a lot, I think they should remain disabled for regular unit testing runs.