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 version of Gram matrix #55

Closed
JeffFessler opened this issue Dec 18, 2021 · 4 comments
Closed

Toeplitz version of Gram matrix #55

JeffFessler opened this issue Dec 18, 2021 · 4 comments

Comments

@JeffFessler
Copy link
Collaborator

Any plan here to provide the Toeplitz version of the Gram matrix A'A?
If not, I probably will work on it but I want to avoid duplicating efforts underway elsewhere...

@tknopp
Copy link
Member

tknopp commented Dec 18, 2021

We have support for that in MRIReco.jl: NFFTNormalOp
Basically what we use here is that there is a fallback implementation for A'A that an iterative solver is using. However, the operator itself can "overload/specialize" this default operation which you can see here: https://github.com/MagneticResonanceImaging/MRIReco.jl/blob/040d51ee38f50609bead454c262291273eede4db/src/Operators/NFFTOp.jl#L88
So in that example the NFFTOp has the option to either return the NormalOp default implementation or to use the toeplitz version NFFTNormalOp.

Regarding where this code should live, I am somewhat unsure. SparsityOperators is our package where we general operators in. We could move NFFTOp there but SparsityOperators probably needs a better name at some point.

By the way: https://github.com/MagneticResonanceImaging/MRIReco.jl/blob/040d51ee38f50609bead454c262291273eede4db/src/Operators/FieldmapNFFTOp.jl is the implementation of this enlightening paper: https://ieeexplore.ieee.org/document/1495877

Finally, I need to mention that I think our implementation is only 2D. So a generalization to at least 1D and 3D would be definitely appreciated. I am right now not working in that area, so no rest of duplicated effort right now.

@JeffFessler
Copy link
Collaborator Author

Thanks for the detailed reply and the nice shout out :)
I need 3D so I plan to work on the N-D case in early 2022.
My thought is that the functions that plan and apply the Toeplitz version (without B0) would go nicely here in NFFT.jl as base methods and then users can choose separately which operator wrapper to use (because I like to use LinearMapsAA). We can discuss more when I actually have some code...

Note to self:
https://github.com/MagneticResonanceImaging/MRIReco.jl/blob/master/src/Operators/NFFTOp.jl
https://github.com/MagneticResonanceImaging/MRIReco.jl/blob/master/src/Operators/FieldmapNFFTOp.jl

@tknopp
Copy link
Member

tknopp commented Dec 18, 2021

Yes, that sounds like a very good plan. It fits nicely into the current concept of keeping the operators out of NFFT.jl but provide all the means to build them in a trivial fashion. I will certainly upgrade our Ops when we have support for the Toeplitz trick in NFFT.jl.

@tknopp
Copy link
Member

tknopp commented Jan 29, 2022

This was implemented in #57. Please reopen, if there are issues.

@tknopp tknopp closed this as completed Jan 29, 2022
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