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

Feature Request: Allow Odd N #36

Closed
JeffFessler opened this issue Jun 12, 2019 · 5 comments
Closed

Feature Request: Allow Odd N #36

JeffFessler opened this issue Jun 12, 2019 · 5 comments
Labels

Comments

@JeffFessler
Copy link
Collaborator

Does NFFT require that N be even?
I am getting incorrect results in both 1D and 2D when N is odd.
(I earlier thought that there was a bug in 2D compared to 1D but later I realized that I was using even N for testing in 1D and odd N for testing in 2D and when I went back and tried odd N in 1D I got the same errors.)
I can make a MWE example if needed, but if it is a known limitation then it would be best for NFFTPlan to report an error for odd N.

@tknopp
Copy link
Member

tknopp commented Jun 13, 2019

I take this as a bug report. Thanks for filing.

@tknopp
Copy link
Member

tknopp commented Dec 30, 2021

For the record, this is definitely a known limitation and on the tk/multithreading branch I already integrated some checks so that this results in an error rather than a wrong result. The NFFT3 lib has this limitation has well, so we have at least feature parity.

The main reason for this limitation is just the fftshift being required. We (and the NFFT3 lib) do this within the apodization matrix which allows us to skip an explicit shift. But I right know don't get the indexing right in my head to actually tackle this.

I still keep the issue open, if someone wants to tackle it.

@tknopp tknopp changed the title Any known issue when size N is odd? Feature Request: Allow Odd N Dec 30, 2021
@tknopp tknopp mentioned this issue Dec 31, 2021
@tknopp tknopp added the feature label Jan 30, 2022
@tknopp tknopp closed this as completed in 4c54224 Nov 7, 2022
@tknopp
Copy link
Member

tknopp commented Nov 7, 2022

This was easier than I thought. Now I am happy that we don't lack behind FINUFFT (and ducc0) anymore :-)

@JeffFessler
Copy link
Collaborator Author

Yay! Looking forward to a version bump so that I can use this elsewhere with an appropriate compat entry.

@tknopp
Copy link
Member

tknopp commented Nov 7, 2022

Release is triggered: https://github.com/JuliaMath/NFFT.jl/issues?q=is%3Aissue+is%3Aclosed+sort%3Aupdated-desc
It will be a patch release since technically, this is not a breaking change. The version will be 0.13.3.

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

No branches or pull requests

2 participants