search

imphys / MRF_undersampling_optimization

GNU General Public License v3.0
4 stars 0 forks source link

Could there be a bug in the numerical jacobian calculatation? #1

Closed fzimmermann89 closed 1 year ago

fzimmermann89 commented 1 year ago

First of all, thank you for providing the source code!

While skimming through the code, I noticed https://github.com/imphys/MRF_undersampling_optimization/blob/3d462202ea6a05a54f01e2bb810b05aeb4dfb896/UEE_phase.py#L690 Doesn't this constitute a bug? The assignment does not produce a copy, but just a new referenceto the data. So for each direction, the step is added to the same data. Instead, for each direction it should be added to the original data. I think that there should be an explicit copy. I might be missing something, but as is, this seems to result in a wrong jacobian.

Have a great day, Felix

DavidHeesterbeek commented 1 year ago

Dear Felix,

Indeed you are right, this was a rookie Python mistake. Thanks for pointing it out. We fixed this problem now. We have checked the convergence using the correct jacobian and the wrong one, and found out that this doesn't change the result. Even the convergence time doesn't change a lot.

Thanks again for your keen eye, David

fzimmermann89 commented 1 year ago

Yes, for small a small stepsize this should not effect the results, as it converges to the same limit.

But thanks for fixing it, and thank you for the interesting work!