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

[SHORT] Fix up codes for FFT class, making it more user friendly and fix grid search #111

Merged
merged 8 commits into from
Jan 13, 2021

Conversation

chaithyagr
Copy link
Contributor

@chaithyagr chaithyagr commented Dec 16, 2020

This PR Covers:

  1. Option for user to send mask directly
  2. Using axes parameter of np.fft.fftn to do FFT in vectorized way, than using loops
  3. Send n_coils to parameters in grid search

Copy link
Contributor

@zaccharieramzi zaccharieramzi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some questions

mri/operators/fourier/cartesian.py Show resolved Hide resolved
for ch in range(self.n_coils)])
axes = tuple(np.arange(1, img.ndim))
return self.mask * np.fft.ifftshift(
np.fft.fftn(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you check (and if so how) that fftn is indeed faster than the loop?

Indeed I had this problem, summarized in this SO question a long time ago.

Finally, it might be nice to consider some parallelization on this (maybe for now as a TODO).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, there was a TODO for parallelization, but then I assumed that the fftn through numpy might as well handle this well and in practice this should not be that big a gain, but you do have a valid point. I will check this and get back here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

moved to scipy

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also added n_jobs and connected the argument to workers. This way, user has control on exact speed

Copy link
Contributor

@zaccharieramzi zaccharieramzi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One other test is failing for a matching problem, you should investigate to see if it's serious or not

mri/operators/fourier/cartesian.py Show resolved Hide resolved
mri/operators/fourier/cartesian.py Show resolved Hide resolved
@chaithyagr chaithyagr changed the title [SHORT] Fix up codes for FFT class, making it more user friendly [SHORT] Fix up codes for FFT class, making it more user friendly and fix grid search Jan 12, 2021
@chaithyagr
Copy link
Contributor Author

Looks like scipy for some reason gives different result, causing issues when we compare Noncartesian to cartesian in some tests
Currently I have added relative tolerance of 1e-3 for it

@chaithyagr
Copy link
Contributor Author

Also, I had a minute issue in gridsearch scripts which came up while using it, the n_coils term is missing and since it is a single line, I am merging it to this PR

@zaccharieramzi
Copy link
Contributor

LGTM @chaithyagr , I'll let you merge when you think it's time

@chaithyagr
Copy link
Contributor Author

merging, I see that the code worked on circleci well.

@chaithyagr chaithyagr merged commit 7b24267 into CEA-COSMIC:master Jan 13, 2021
@chaithyagr chaithyagr deleted the fft_gpu branch January 15, 2021 09:20
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.

None yet

2 participants