I won't be in a position to test this this week still, but I would really, really want to see RC2 happen this week though... it's been delayed already quite a bit as is. So if anyone else can give this a test, that'd be great. Even if not, I'm not against having it in RC2, maybe with a "beta" kind of warning to go with it? As we all know, the best testing is that from the whole user base. We've often had features tested quite well, only for a user to highlight a bug/problem on day 1 after release of said feature. 😐

I'm not against having it in RC2, maybe with a "beta" kind of warning to go with it? As we all know, the best testing is that from the whole user base. We've often had features tested quite well, only for a user to highlight a bug/problem on day 1 after release of said feature.

Yes, I'm leaning that way too. I think it works fine, and the output seems to match the original authors' unring binary. Visually, the output is equivalent, but there are numerical differences, sometimes quite large, between the two implementations. That said, we are using a different FFT library, we do use the correct value of PI (?), and I have removed a couple of redundant steps. Not sure how to verify that the output is valid when it's not an exact match to the reference implementation... But I agree that a beta release might be a good compromise.

Any other thoughts...?

OK, added a couple more enhancements - we can now link against FFTW. This results in equivalent runtime to the authors' original unring implementation (in single-threaded mode) with identical output (within the precision of the data type: unring produces 32-bit integer NIfTIs on my system). A 68 volume DWI dataset gets de-Gibbs-ed in ~7s on my system with FFTW, about 14s using the default kissFFT implementation in Eigen.

I'll add a test, and then we should be good to go.

One final note: are we all OK with the name mrdegibbs...?

Haven't tested myself, but if you can (almost) identically reproduce unring's output, that's a good sign.

Just raising some points for thought:

• Currently it's locked to operate in 2D. How is people's understanding of how this works in the presence of partial Fourier? I think the paper said it can still be applied but the correction is "incomplete"; but I don't know for sure if it can be detrimental to apply in cases with severe partial Fourier fractions / k-space filtering, in which case you may want to run it in 1D. Also theoretically it could be applied to 3D FFT data (though I don't understand the technique enough to judge how difficult it would be to extend to 3D).

• Could possibly do with a comment in the DESCRIPTION field regarding that this can't be applied after any kind of image interpolation has taken place. It's described in the manuscript, but the command could maybe do with an upfront awareness campaign.

• Currently the default subvoxel spacing resolution is 20, same as unring. I only did a little testing when I first tried unring but I'm pretty sure I noticed a difference with a higher resolution, and bumped it up to 100 in the connectome script. It may take longer than 7 seconds, but in the context of all of the other image processing that needs to take place, it's not the end of the world.

• Was the test data generated using unring or mrdegibbs? It would be kinda cool to directly save a .nii.gz from unring, and use that as the testing target.

• Content with the name mrdegibbs. Think it's sufficiently descriptive and accurate that an acronym e.g. mrgrc isn't necessary. We might have settled on a new "component" of command naming i.e. data de unwanted_effect.

• Currently it's locked to operate in 2D. How is people's understanding of how this works in the presence of partial Fourier? I think the paper said it can still be applied but the correction is "incomplete"; but I don't know for sure if it can be detrimental to apply in cases with severe partial Fourier fractions / k-space filtering, in which case you may want to run it in 1D. Also theoretically it could be applied to 3D FFT data (though I don't understand the technique enough to judge how difficult it would be to extend to 3D).

Good point about adding documentation for the handling of partial Fourier acquisitions - I'll do that in a bit. However, running this in 1D or 3D is going to be quite a bit of work, and require fairly extensive modifications, particularly to the filtering that's applied initially. I'm not sure there's any discussion of that in Kellner's original paper, but I suggest that if they don't cover it, we should leave it well alone - maybe leave it as a topic for future research for a Master's student...? I note there is discussion of the 1D case (since that's essentially what's applied to each row & column in the image in the 2D case, but no mention of the appropriate filtering to perform 1D unringing on a 2D image. Personally, I think we should stick as close as possible to the original unring implementation, and only extend it if there is a clear need for it.

• Could possibly do with a comment in the DESCRIPTION field regarding that this can't be applied after any kind of image interpolation has taken place. It's described in the manuscript, but the command could maybe do with an upfront awareness campaign.

Good suggestion, will do. Should also mention that this should probably run after dwidenoise...?

• Currently the default subvoxel spacing resolution is 20, same as unring. I only did a little testing when I first tried unring but I'm pretty sure I noticed a difference with a higher resolution, and bumped it up to 100 in the connectome script. It may take longer than 7 seconds, but in the context of all of the other image processing that needs to take place, it's not the end of the world.

OK, the shifts clearly will make a difference, but I didn't find the difference to be particularly appreciable visually even going down as low as 10 on my run-of-the-mill data... The processing time will scale almost linearly with the number of shifts, so I wouldn't make 100 the default unless there was solid evidence that the difference it makes is non-negligible. Personally, I find it hard to believe that a granularity of 1/20th of a voxel isn't sufficient. I'd personally rather stick with the original authors' implementation defaults here, which they presumably chose with good reason, and modify if that proves insufficient.

• Was the test data generated using unring or mrdegibbs? It would be kinda cool to directly save a .nii.gz from unring, and use that as the testing target.

Not the test data, no. I used the small cropped dwi.mif image as the input to the test, and for some reason that does lead to appreciable differences in the outputs. Might be a good idea to add a full size b=0 single volume and test on that, the execution time is fast enough that it won't be an issue. And we could then use the output of unring directly - although that will entail dropping the tolerance down to ~1 to account for the integer representation in unring. Unless providing a floating-point input means it'll produce a floating-point output...? I'll check.

• Content with the name mrdegibbs. Think it's sufficiently descriptive and accurate that an acronym e.g. mrgrc isn't necessary. We might have settled on a new "component" of command naming i.e. datade unwanted_effect.

Well, I assumed you would be, given that's the name you suggested in #715... 😉

OK, that's all done. I reckon we can merge...?

Ok, looks like we've got sufficient approval here. This should also not affect any other existing functionality, and we're not actively recommending the step yet in any pipeline (i.e. in the userdocs), so I reckon it's safe to be in "pretty-well-tested-beta". Let's merge now.