JuliaImageRecon / RegularizedLeastSquares.jl

MIT License
20 stars 9 forks source link

[breaking] Remove ConstraintTransformRegularization #70

Closed JakobAsslaender closed 7 months ago

JakobAsslaender commented 8 months ago

@tknopp, as discussed, I implemented the ADMM and the SplitBregman to take reg and regTrafo as separate arguments instead of merging them in a ConstraintTransformRegularization type. I further added documentation, to highlight this difference to the ISTA-type algorithms.

Another little change I made on the way is to remove the vec(reg) function, as I find a a bit of an overkill and hard to read. Not sure, if it also boosts the need for recompilation since we're dispatching a Base function. I declared the function for now as deprecated, so this particular change is not (yet) breaking.

As always, any comments are welcome!

nHackel commented 7 months ago

I am fine with moving the regTrafo back to the solver, especially with the added solver documentation. The ConstraintTrafo was a bit of an odd one out in the decorater pattern of the regularization terms, as it didn't really add functionality to a term/only added in the context of those specific solvers.

I think maybe we shouldn't deprecate createLinearSolver and instead try to make this a "helpful" constructor that checks if the given arguments are applicable for a solver. If they aren't it could still construct the given solver but also produce a warning with a list of applicable alternatives. Lastly we could also have a createLinearSolver without a given solver type, that just returns the first fitting solver (there are questions about priority here then, but this whole paragraph is more for different PRs)