search

NYU-DiffusionMRI / mppca_denoise

28 stars 13 forks source link

mpdenoise, line 129, "if M > N" has become "if M < N". Why? #3

Open hex808080 opened 1 year ago

hex808080 commented 1 year ago

I have noticed that the latest commit of mpdenoise.py has changed the direction of the if statement at line 129 from "if M > N" to "if M < N". Is there a reason behind this change?

Our group is using code based on the 2020 commit, i.e. "if M > N". To what extent updating to the newest version of mpdenoise will affect our new results? Will new results still be compatible/comparable with those produced so far with the 2020 commit code?

badesar1 commented 1 year ago

I think it's just a change in the notation. In Jelle's matlab code you can see the M refers to the number of diffusion measurements and N refers to the patch size, in my code I swapped the notation such that M refers to patch size and N refers to the number of measurements.

I do agree that ideally the notation should be consistent, but you should have nothing to worry about from the perspective of performance.

Best, Ben

hex808080 commented 1 year ago

Please correct me if I'm wrong, but I don't think this is just a matter of changing notation. The definition of M and N has stayed the same across commits:

sx, sy, sz, N = self.dwi_tmp.shape # N = number of DW volumes
...
M = np.prod(self.kernel).astype(int) # M = unwrapped kernel patch length

Now, let's consider some data with 80 DW directions, and default 5x5x5 kernel patch. For all versions of mpdenoise.py, M and N will be:

N = 80
M = 125 # = 5x5x5

In commit d748e0b: if M > N, so the code will enter the if statement and the boxpatch matrix X will be transposed. In commit 45cf8fb: if M < N, so the code will NOT enter the if statement and the boxpatch matrix X will NOT be transposed.

Same data, different behaviours, which will affect subsequent calculations of eigenvalues, gamma, and so on.

This is why I do not believe this is just a matter of different notation (if M and N were also inverted when defined, i.e. prior to calling the denoise method, then it would be).

So my question is: is this a mistake and, if not, how different the final results will be when feeding the same data to the two commits?

badesar1 commented 1 year ago

I see what you are referring to now. The original code did have a bug that was fixed in 2021 with the newer commit. As far as I could tell the major impact this bug had was in the estimation of the noise level sigma. The denoising performance did also improve after correcting the bug, but I have not quantified the degree. I can assure you that you do want to be using the newest version to ensure that the noise level is estimated properly.