PTB-MR / mrpro

MR image reconstruction and processing.
https://ptb-mr.github.io/mrpro/
Apache License 2.0
17 stars 2 forks source link

Should IterativeSense have regularization? #369

Closed fzimmermann89 closed 1 month ago

fzimmermann89 commented 4 months ago

Should Iterative sense have regularization parameters in the __call__ ? Or, alternatively, should this be a separate Reconstruction class, called RegularizedIterativeSENSE?

@fzimmermann89 and @ckolbPTB were in #287 for adding the parameters and keeping the name IterativeSENSE.

@koflera suggested a different class for this, as it is a different type of reconstruction.

My suggestions would be to allow in __call__ to provide a lambda (defaults to 0) and a x_reg (default to None) If lambda>0, it is tikonov regularization with lambda||x||^2, if x_reg is also set, it performs lambda||x-x_reg||^2 The additional code in the reconstruction is minimal and would avoid having to duplicate much of the existing code.

A separate class would mainly copy the same stuff. If you want to, you could have a IterativeSense inheriting from RegularizedIterativeSense and setting lambda to 0... But having a separate class for this seems overkill imho.

Do you have any other opinions?

koflera commented 4 months ago

Thanks for summarizing this.

Just to explain a little bit more in detail why I don't like the first variant.

As far as I got it, we looked up the iterative SENSE paper and there, no regularization is used. Since setting \lambda> 0 is more general, I would instead have a RegularizedIterativeSENSEclass, for which you can set \lambda=0, T=Id and reg=None.

I just find it unintuitive to have IterativeSENSE (whose name historically suggests that it solves 1/2 * ||Ax-y||_2^2) that is able to solve 1/2 * ||Ax-y||_2^2 + \lambda/2 * ||Tx - reg||_2^2, while having RegularizedIterativeSENSEthat is also able to solve IterativeSENSEmakes more sense to me.

So my opinion is to either have Only the RegularizedIterativeSENSEor two separate RegularizedIterativeSENSE and IterativeSENSE, instead of only IterativeSENSE.

schuenke commented 4 months ago

No strong opinion here, but I kind of agree with @koflera and would vote for two separate classes IterativeSENSE and RegularizedIterativeSENSE. But like I said: I don't care to much 😁

ckolbPTB commented 2 months ago

Final decision: IterativeSENSE with comment that RegularizedIterativeSENSE is available and only has init with lamba = 0