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

Issue with convolution over pixel using astropy.convolution.Box2DKernel? #754

Open
schlafly opened this issue Oct 12, 2023 · 4 comments
Open
Assignees

Comments

@schlafly
Copy link

I've been doing some Roman simulations and comparing the input simulated positions with the measured positions using photutils and the webbpsf gridded_psf routines. I eventually noticed that I got good performance if I picked odd oversampling factors and bad performance (~0.01 systematic pixel phase errors) if I used even oversampling factors.

I think I understand the root cause to be the convolution over the oversampled pixel around here
https://github.com/spacetelescope/webbpsf/blob/develop/webbpsf/gridded_library.py#L260
For the odd case, this is easy and just sums over the N oversampled pixels. For the even case, it's a bit more complicated since if you just sum over the N oversampled pixels you introduce a centroid shift of half a pixel, since you can't center the even array right on a pixel. The astropy convolution machinery is dealing with this somehow, but I haven't looked to see exactly what they're doing. But I don't think they're doing a great job for kernels like this one that are discontinuous.

If I manually replace the PSFs generated by gridded_library with my own versions where I do a different convolution (a la the convolve_scipy here https://gist.github.com/schlafly/a50a27411bdc706c69e093d083a4b6db), then I get good performance for both odd and even oversampling rates.

That gist has a little demo that produces output like

over   astro    scipy
001 0.014922 0.014922
002 0.006254 0.002840
003 0.001345 0.001343
004 0.001656 0.000729
005 0.000445 0.000437
006 0.000788 0.000312
007 0.000207 0.000187
008 0.000484 0.000175
009 0.000123 0.000083
010 0.000344 0.000122
011 0.000096 0.000030
012 0.000270 0.000102
013 0.000091 0.000000

showing that the difference between the astropy convolution and "truth" has this oscillating pattern where oversample 4 is worse than oversample 3, etc.. Meanwhile the scipy version I cooked up is better but starts oscillating around oversample 10. Note that the two versions agree closely for odd oversampling smaller than 7; they're doing the same thing. But the even oversampling is a bit different.

I worry I might not have fully tracked down everything there; I feel like for my intended band-limited 'truth' image I should have seen much better convergence than I did---but I think this is still a good illustration of the issue.

I think I might recommend doing something different for the convolution in the even sampling case, along the lines of my convolve_scipy routine, but you may have better ideas.

@Skyhawk172
Copy link
Collaborator

I think that's another way of showing the same thing using Eddie's approach in his gist: for odd oversampling, the Scipy and Astropy convolutions give (visually) the same things, whereas even oversampling yields to sharper differences.

image

@schlafly
Copy link
Author

schlafly commented Feb 8, 2024

In my head I was guessing that astropy is replacing the even kernel with some half-pixel shifted odd kernel, and so replacing the top-hat with something where the wings ramp linearly down or something, which sounds sane but for a discontinuous kernel like this one would be problematic. But I didn't push hard enough to actually find the code that was doing that and may be off-base. That would make the effective kernel a bit wider than it should be for even oversampling and have an effect like the one we see here.

@Skyhawk172
Copy link
Collaborator

Right - as @obi-wan76 pointed out, Astropy Boxkernel2D returns an odd-size kernel regardless of the width specified, so that's one difference with the Scipy approach used above.

With that said, I don't measure any difference in the centroids of the images I posted above...

@schlafly
Copy link
Author

schlafly commented Feb 8, 2024

Ah, sorry, I'm not sure I can see obi-wan76's message, thanks. Yes, I didn't test centroid differences for my test image, only for the real Roman images, but you can imagine that I was surprised when I learned that I could reproduce the input centroids well for Roman only for odd-sized convolutions! Note that the systematics were small (~0.01 pix), but we obviously shouldn't be limited by this particular systematic. The real Roman PSFs are of course a bit more complicated than these test images.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants