Hi @yanielc ,

Thanks for your implementation of this new transform. I have 2 minor comments:

1. Could you please help add the new transforms to the docs page: https://github.com/Project-MONAI/MONAI/blob/dev/docs/source/transforms.rst
2. Do you plan to also implement the dict version of these transforms?

Thanks.

Hi @yanielc ,

Thanks for your implementation of this new transform. I have 2 minor comments:

1. Could you please help add the new transforms to the docs page: https://github.com/Project-MONAI/MONAI/blob/dev/docs/source/transforms.rst
2. Do you plan to also implement the dict version of these transforms?

Thanks.

Hi @Nic-Ma,

I plan to add the dictionary style versions in this pull request as well. I just wanted to get some feedback before adding them all.

I will add the four transforms to the documentation pages.

Thanks.

There are a few transforms now that operate in k-space (including your previous Gibbs transform). Do you think you could abstract out the FFT and iFFT to a common function to reduce code duplication?

You may also remember that in your Gibbs PR, you originally used torch for the FFT/iFFT, but I recommended that we switch to numpy because the functionality didn't exist in older versions of PT. I've since learnt that elsewhere in MONAI (e.g., HilbertTransform), we use the torch version and then in the testing, we skip if the functionality isn't present. Might be worth bearing in mind if we unify all FFT/iFFT functionality.

There are a few transforms now that operate in k-space (including your previous Gibbs transform). Do you think you could abstract out the FFT and iFFT to a common function to reduce code duplication?

You may also remember that in your Gibbs PR, you originally used torch for the FFT/iFFT, but I recommended that we switch to numpy because the functionality didn't exist in older versions of PT. I've since learnt that elsewhere in MONAI (e.g., HilbertTransform), we use the torch version and then in the testing, we skip if the functionality isn't present. Might be worth bearing in mind if we unify all FFT/iFFT functionality.

I was thinking the same about a possible super class for the Fourier transforms. I would not mind writing the code and rewriting the Gibbs and Spikes transforms to inherit from the new class. To compartmentalize the work more, can I make those changes on a different pull request right after this PR is done?

Thanks for the heads up on skipping the functionality option for the older Pytorch tests.

Hi @Nic-Ma and other reviewers, just letting you know that the PR is ready for review.