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

Toeplitz gram #57

Merged
merged 14 commits into from
Dec 31, 2021
Merged

Toeplitz gram #57

merged 14 commits into from
Dec 31, 2021

Conversation

JakobAsslaender
Copy link
Contributor

Hi @tknopp , @JeffFessler ,

re #55 : I gave it a first shot. Right now, the constructor is doing

fft(fftshift(apodization_adjoint_with_fftshfit(ifft(x)))

From my understanding, the apodization function is doing an ifftshift. I am not sure if it is possible to perform the apodization without this, I am having a hard time reading the metaprogramming.

Apart from that, I tested it with different data types and N dimensions.

Let me know what you think, I am sure both of you have a much better suggestion on how to implement this, I am somewhat an NFFT rookie.

Best,
Jakob

@JakobAsslaender
Copy link
Contributor Author

Hmm, I incorporated Polyester.jl into the LUT generation. That does not seem to fly with Julia 1.3. If you want to keep the compat, we could switch to Threads.@threads, but it was slower in my brief tests. On the other hand, Julia 1.6 is now the LTS, so maybe we can drop the older version?

@JakobAsslaender JakobAsslaender mentioned this pull request Dec 28, 2021
@tknopp
Copy link
Member

tknopp commented Dec 29, 2021

yes we can bump to Julia 1.6!

@codecov
Copy link

codecov bot commented Dec 29, 2021

Codecov Report

Merging #57 (11dd913) into master (e5c18d5) will increase coverage by 0.39%.
The diff coverage is 76.42%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #57      +/-   ##
==========================================
+ Coverage   67.34%   67.73%   +0.39%     
==========================================
  Files          12       13       +1     
  Lines         692      750      +58     
==========================================
+ Hits          466      508      +42     
- Misses        226      242      +16     
Impacted Files Coverage Δ
src/CpuNFFT.jl 70.08% <62.90%> (-7.06%) ⬇️
src/NFFT.jl 53.48% <66.66%> (ø)
src/Toeplitz.jl 91.30% <91.30%> (ø)
src/precomputation.jl 69.76% <91.66%> (-1.17%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e5c18d5...11dd913. Read the comment docs.

@tknopp
Copy link
Member

tknopp commented Dec 30, 2021

Unfortunately our two branches diverged because we touched both too many files. We should fix this quickly which unfortunately requires some work. I would say we should go this way

  1. merge my tk/multithreading branch
  2. isolate the Toeplitz stuff into a new branch (you can create it here directly in this repo)
  3. get your other changes in (docu and so on)

In the future we can then directly work against master and gradually improve that. I would like the Toepliz stuff isolated on an extra branch so that Jeff can first review that.

@JakobAsslaender
Copy link
Contributor Author

JakobAsslaender commented Dec 30, 2021

Just saw that you merged it. Sure, I will merge master into my branch and make sure everything is works again. And I agree, Jeff's feedback would be great and he offered to do so via email.

@tknopp
Copy link
Member

tknopp commented Dec 30, 2021

Done, sorry for the merge conflicts! I duplicated some of your work since I needed to introduce a new precompute flag FULL_LUT, which reduces the precomputation time for the FULL flag substantially.

@JuliaMath JuliaMath deleted a comment from JakobAsslaender Dec 31, 2021
@tknopp
Copy link
Member

tknopp commented Dec 31, 2021

I merge this as is since it touches too many files and we can gradually improve it on master. @JeffFessler can do a post merge review.

Regarding the apodization: Yes, right now it is doing an fftshift and it is not possible to disable this. But we can simply implement an additional version without that. This should also allow us to have an NFFT for odd N (see #36).

@tknopp tknopp merged commit 101d9d9 into JuliaMath:master Dec 31, 2021
@JakobAsslaender JakobAsslaender deleted the ToeplitzGram branch December 31, 2021 17:04
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

Successfully merging this pull request may close these issues.

2 participants