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

Missing ifftshift in filter code? #2399

Closed
smartalecH opened this issue Feb 14, 2023 · 2 comments
Closed

Missing ifftshift in filter code? #2399

smartalecH opened this issue Feb 14, 2023 · 2 comments

Comments

@smartalecH
Copy link
Collaborator

I think we are missing an ifftshift in the adjoint filter code:

(kx, ky) = x.shape
h = _proper_pad(h, 3 * np.array([kx, ky]))
h = h / npa.sum(h) # Normalize the kernel
x = _edge_pad(x, ((kx, kx), (ky, ky)))
return _centered(
npa.real(npa.fft.ifft2(npa.fft.fft2(x) * npa.fft.fft2(h))), (kx, ky)
)

per the discussion we had awhile back.

While writing a 1D variant of this, I noticed that the _centered function ends up grabbing the ends of the filtered array (i.e. all the padding) because we never shifted the origin... but maybe numpy's 2D fft has a different convention?

(cc @mawc2019, @mochen4)

@mawc2019
Copy link
Contributor

I think we are missing an ifftshift in the adjoint filter code

I do not think fftshift or ifftshift is missing. Here the kernel is padded with zeros at the center, in contrast to the previous case (before PR #2016) where the kernel is padded with zeros at ends.

While writing a 1D variant of this, I noticed that the _centered function ends up grabbing the ends of the filtered array (i.e. all the padding) because we never shifted the origin... but maybe numpy's 2D fft has a different convention?

Are you referring to the case before or after PR #2016? After PR #2016, the 1d variant was used, but fftshift or ifftshift was not used. Before PR #2016, the 1d variant was not used, but fftshift was used.

@smartalecH
Copy link
Collaborator Author

Ah yes you're right. I forgot we padded the center, not the edges. Thanks!

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

2 participants