@yanielc thanks! Do you think you could tidy up the git diff? I'm not sure why, but the changes from this PR seem to be present. Perhaps simply clicking "Update branch" will solve it.

Would you also be able to attach a before and after photo in this thread so we can visually check the transform?

Would you also be able to attach a before and after photo in this thread so we can visually check the transform?

Hi @rijobro, here one can see the transform applied on a sample image from the BraTS dataset.

Leftmost is the original volume, then separate applications of the transform with different strengths.

@yanielc thanks! Do you think you could tidy up the git diff? I'm not sure why, but the changes from this PR seem to be present. Perhaps simply clicking "Update branch" will solve it.

Sure thing, I can try. I also noticed afterwards that my pull request had stuff from other PR you mentioned. I'll try your suggestion.

@yanielc let me know when you want me to re-review this.

@yanielc let me know when you want me to re-review this.

@rijobro, I'm done with the commits, it's ready now. I tried to address all your suggestions.

Aside from the remaining comments, unit tests are needed for all 4 classes (random/deterministic and array/dictionary). Should use a variety of inputs (alpha as float vs. list) etc. Should also be tested on both 2d and 3d images with a variety of number of channels.

Sounds good, I will commit the changes possibly tomorrow and I will include the unit tests.

Aside from the remaining comments, unit tests are needed for all 4 classes (random/deterministic and array/dictionary). Should use a variety of inputs (alpha as float vs. list) etc. Should also be tested on both 2d and 3d images with a variety of number of channels.

Hi Richard, the commit is ready for review. I added the unit tests as well.

Looks really good, just need to get all the checks green and then I think this is good to go. Thanks!

One thing I was assuming, perhaps unnecessarily, was that the number of leading dimensions was arbitrary, say (..., channel, H, W) for 2D and (..., channel, H, W, D) for 3D. Hence my mask method was a bit less direct.

I think all of our transforms assume (C, H, W, [D]) with no batch. I think this is a safe assumption here as otherwise all our other transforms would fall over.

Looks really good, just need to get all the checks green and then I think this is good to go. Thanks!

There are currently two tests failing. One is build-docs. The interesting thing is that when I run make html locally, I no longer see any warnings or errors. So I am a bit uncertain what to do about the error it shows on the repository:

/home/runner/work/MONAI/MONAI/monai/transforms/intensity/array.py:docstring of monai.transforms.intensity.array.GibbsNoise.shift_fourier:1: WARNING: duplicate object description of monai.transforms.GibbsNoise.shift_fourier, other instance in transforms, use :noindex: for one of them

The other error is simpler. It's flake8. When I run it locally it wants to reformat more files than the ones I have edited it. Can I let it do that?

Thank you for all guidance!

@wyli @Nic-Ma @ericspod – do you think we could revisit the use of DCO? I committed from a github suggestion, which apparently doesn't do the DCO (so not very useful), but I can't force push since it's not my branch...

@wyli @Nic-Ma @ericspod – do you think we could revisit the use of DCO? I committed from a github suggestion, which apparently doesn't do the DCO (so not very useful), but I can't force push since it's not my branch...

there is a discussion about the DCO here dcoapp/app#141

@rijobro, almost all green marks! I think you saw there is just one missing sign-off from a previous commit of yours.

By the way, I noticed that the docs are showing every method of the Gibbs transforms, for example inv_shift_fourier and apply_mask. I think I should rename them to private methods so that they don't show up in the docs.

tell me when you're happy. I'll have to manually set the DCO check to pass, and then enable auto-merge.

tell me when you're happy. I'll have to manually set the DCO check to pass, and then enable auto-merge.

Ok, I think I am done with changes on my end.