FixSplitBregman

[JakobAsslaender]

Hi, I made the following changes:

• bugfix: push!(regTrafo, trafoReg)-> push!(regTrafo, trafoReg.trafo) in ADMM and SplitBregman
• other bugfixes in SplitBregman relating to the size of dual variables
• unify the notation of ADMM and SplitBregman
• half lambda in both algorithms to match the regularization strength w/ ISTA-type algorithms

Both solvers should work now as expected, but I have two questions:

• is the intention of SplitBregman to be used for "constraint problems," i.e., for solving ||G x||_1, subject to ||Ax - b||_2^2 < sigma? The only real difference I can see between the algorithms is this outer loop. The other implementation difference is the scaling of rho, which is scaled to match the corresponding papers, but this could also be changed to macth between the algorithms.
• For both algorithms to work appropriately, it seems neccessary to call them with a ConstraintTransformedRegularization, which feels a little unituitive to me. Would you be so kind to explain to me intended difference between the different nested regularization terms–and their relation to the unnested ones?

As always, any feed is more than welcome!

[JakobAsslaender]

Closes #43

[JakobAsslaender]

Just kidding. I matched rho in SplitBregman to the one in ADMM as the tests failed when due to rho/lambda w/ lambda = 0.

[tknopp]

@migrosser

[JakobAsslaender]

@migrosser, @tknopp , a friendly ping if you have any thoughts on this PR and the questions raised above.

Thanks!

[tknopp]

For these algorithms we have a little maintainability issue since I am not deep enough in the proximal algorithms. It's a little bit unclear if @migrosser can have a look, since he finished his PhD and now took on another opportunity.

But I try to summarize a discussion which @nHackel @migrosser and once had. Initially, we thought that it would be sufficient to just have the transform within the regularizer and not in the data discrepancy term. But if I understood it correctly (?), this requires a unitary transform since one needs to apply the inverse (see https://github.com/JuliaImageRecon/RegularizedLeastSquares.jl/blob/master/src/Regularization/TransformedRegularization.jl#L31). For the ConstrainedTransformedRegularizeation, we however, can apply the transform itself. That's the reason why we have both and the the user needs to decide, which to use.

[tknopp]

But to get forward, I am going to merge this now. Tests are apparently passing.

[JakobAsslaender]

Thanks, @tknopp for the explanation. It makes sense that we cannot use TransformedRegularization and the current setup with ConstraintTransformedRegularization works. I wonder if it was more transparent to the user to write the ADMM interface to take a transformation and a naked L1-regularization as arguments and to remove the ConstraintTransformedRegularization concept. It took me several hours of paper reading etc. to figure out that this pseudo-nested regularization type is needed for ADMM to work properly and I believe that exposing the difference to the user might help them use the algorithm correctly. Happy to create a PR if you think this is a good idea.