vais-ral / CCPi-FrameworkPlugins

Apache License 2.0
0 stars 2 forks source link

prox syntax and spelling #16

Closed mehrhardt closed 5 years ago

mehrhardt commented 6 years ago

https://github.com/vais-ral/CCPi-FrameworkPlugins/blob/5b0077ea9531f5212d82868eeb63a9a574479594/Wrappers/Python/ccpi/plugins/regularisers.py#L66

I have two issues with this line.

  1. The prox operator should have an input parameter "sigma" or "tau" or "lambda" which is the "step size" of the prox operator. The prox operator of a function "sigma f" is defined as the operator that maps "z" onto prox_{sigma f}(z) := \argmin_x \frac12 |x - z|^2 + sigma * f.

This step size may be related to the "Lipschitz constant" of some operator but in general it will not be. Thus the naming is misleading. Also, it is very important for the algorithms where this will be used (PDHG, FISTA, etc) that the mathematical formula is implemented correctly (having this correct step size parameter). It would be good to have unit tests for this.

  1. There is a spelling mistake. It is called "Lipschitz" named after https://en.wikipedia.org/wiki/Rudolf_Lipschitz
paskino commented 6 years ago

It's named tau in the Function object.

So the question is: is the parameter called Lipshitz the "step size" (tau) or is it the Lipschitz parameter?

dkazanc commented 6 years ago

@mehrhardt I think you're right, we shouldn't attach the Lipschitz constant (L) to the step-parameter of the proximal operator. A good example would be the ADMM method where "rho" is not necessarily related to L which is used in the inner loop. So for this module I suggest adding "rho" or "tau" and for FISTA case make it equal to L ?

mehrhardt commented 6 years ago

Even for FISTA one may not want it to be equal to the Lipschitz constant. To me that really depends on the user.

dkazanc commented 6 years ago

Indeed, I had a case with FISTA when ordered-subsets and/or non-convex data fidelity iterations were not converging so I had to manually select the parameter. @paskino I'll fix this in the PR

paskino commented 5 years ago

I guess this can be closed