Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[breaking] Remove ConstraintTransformRegularization #70

Merged
merged 2 commits into from
Jan 26, 2024

Conversation

JakobAsslaender
Copy link
Member

@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
Copy link
Member

nHackel commented Jan 25, 2024

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)

@JakobAsslaender JakobAsslaender merged commit eae172c into master Jan 26, 2024
9 of 10 checks passed
@JakobAsslaender JakobAsslaender deleted the RemoveConstraintTransform branch January 26, 2024 16:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants