search

CEA-COSMIC / pysap-mri

MRI external plugin for Python Sparse data Analysis Package
Other
43 stars 18 forks source link

Stacked 3D implementation of NFFT3D #34

Closed chaithyagr closed 5 years ago

chaithyagr commented 5 years ago

This addresses the feature discussed in #33 Also some code cleanups for adjoint tests

LElgueddari commented 5 years ago

Do you think you could also add an example for the stack 3D thing?

chaithyagr commented 5 years ago

Do you think you could also add an example for the stack 3D thing?

Yes, I plan to add one, for stack based reconstruction. (Non Cartesian, using radial)

chaithyagr commented 5 years ago

@LElgueddari. I have a basic example that I formulated, I am having some issues which I will work through, but irrespective, we can get this feature in if it takess too long, especially given that we dont have basic 3D examples yet. For sanity, I have tests in place

chaithyagr commented 5 years ago

I have added an example also. This needed a max_iter for gradient file. It looks like this file is not at all documented. For now, to keep things moving, I have not documented this parameter. We will do this when we refactor the code in #36

chaithyagr commented 5 years ago

General question (probs should have asked earlier but I just realized). Why do we only support Stacked non-uniform 3D Fourier transform?

I dont think I understand this @zaccharieramzi . The reason is speedups and also that it is very memory efficient

zaccharieramzi commented 5 years ago

My question was why don't we also support Stacked 3D (regular) Fourier Transform?

LElgueddari commented 5 years ago

So basically the idea of stack 3d is to take advantage of the Cartesian distribution in the 3D dimension by using FFT instead of NFFT in that direction. For 3D FFT either stack or not the Fourier transform is already doing 1d FFT in each direction. Therefore stack 3D fft is just the basic fftn. Hence there is no point of doing that

philouc commented 5 years ago

My question was why don't we also support Stacked 3D (regular) Fourier Transform?

My answer @zaccharieramzi : because we're supporting 3D acquisitions that encode stack of stars (radial)/spirals/2D sparkling and then along the k_z direction, the sampling is regular so FFT is much more efficent as @LElgueddari outlined

zaccharieramzi commented 5 years ago

I understand that fftn allows us to perform 3D stacked FFT, I am asking why we don't include it. For example we include 2D FFT even if fft2 already allows to do it.

chaithyagr commented 5 years ago

@LElgueddari gave a spot on answer there.. Here is fftn implementation which does exactly the same over all axes:

https://github.com/numpy/numpy/blob/d9b1e32cb8ef90d6b4a47853241db2a28146a57d/numpy/fft/pocketfft.py#L624-L631

chaithyagr commented 5 years ago

I understand that fftn allows us to perform 3D stacked FFT, I am asking why we don't include it. For example we include 2D FFT even if fft2 already allows to do it.

Loubna's PR #51 will generalize FFT2 to FFT class, which would implement FFTN in the same exact fashion. Having a version here would make it redundant

zaccharieramzi commented 5 years ago

So there is absolutely no need for stacked cartesian @philouc ? @chaithyagr like I said, I understand fftn, but we do support fft2 so why not fftn?

chaithyagr commented 5 years ago

So there is absolutely no need for stacked cartesian

Oh we surely need that, and as I mentioned, @LElgueddari 's PR will get that in.. (With a 3D Example for Cartesian Undersampling)

zaccharieramzi commented 5 years ago

@chaithyagr I see thanks, that was the answer I was looking for.

philouc commented 5 years ago

Yes we could add support to 3D Cartesian sampling/undersampling as it is used for instance in 3D EPI for fMRI.

LElgueddari commented 5 years ago

PR #51 already support 3D

chaithyagr commented 5 years ago

All checks have passed! @LElgueddari can you please merge?