add TV operator, merge ProximalGradient and AcceleratedProximalGradient, add GeneralizedProximalGradient

[olivierleblanc]

• I added the TV operator and its proximal operator, inspired by the code in PyUnLocBox.

• I propose to merge ProximalGradient and AcceleratedProximalGradient, because the code is the same, it only requires to choose between None, fista and vandenberghe. If None, the non-accelerated version is used. (Note I intend to merge ADMM and LinearizedADMM the same way later.)

• I also added an implementation of the GeneralizedProximalGradient solver, handling multiple differentiable functions gathered in proxfs and non-differentiable but convex functions with a prox gathered in proxgs.

• I think I added a relevant test for the 1D case as well as associated docstring. I'm not adding a tutorial using these new features for available time issues, but I can certainly guide someone interested into doing so.

• The TV operator still works up to 4D tensors, though.

[olivierleblanc]

Additional comment: I did not consider any backtracking for the GeneralizedProximalGradient.

[mrava87]

Thanks!

These are nice additions :)

A few questions/comments:

• I added the TV operator and its proximal operator, inspired by the code in PyUnLocBox.
  Makes sense. I guess the idea for this one is to use it with solvers of the form f(x)+g(x), right? As in solvers like PD of the form f(x)+g(Kx) we could already do TV as g=L_2,1 and K=Gradient.

• I propose to merge ProximalGradient and AcceleratedProximalGradient, because the code is the same, it only requires to choose between None, fista and vandenberghe. If None, the non-accelerated version is used. (Note I intend to merge ADMM and LinearizedADMM the same way later.)
  Good idea. I think it makes sense for Proximal gradient as there is very little difference in the codes. For ADMM, we should consider this Issue Extend ADMM to general case with A and B #79 and create as many variations as possible that can be solved no matter what choice of A,B,proxf,proxg (eg see ADMML2) and eventually put them all under the same umbrella.

• I think I added a relevant test for the 1D case as well as associated docstring. I'm not adding a tutorial using these new features for available time issues, but I can certainly guide someone interested into doing so.
  Sounds good. It would be nice to take one tutorial where we use isotropic TV via Grad+L_21 with PD (eg https://pyproximal.readthedocs.io/en/latest/tutorials/denoising.html#sphx-glr-tutorials-denoising-py) and show how we can use this new operator with for example ProximalGradient. I can give it a go and ping you when I have something.

Finally, since this PR introduces a few breaking changes (no problem since we are still in version0) I want to make a release first so that people can still use their codes up till now, and then merge your PR as soon as the release is out.

[mrava87]

@reivilo3 I am going through your new TV operator and I am struggling a bit to understand how that could be used with any of our solvers. Since you modified the ProximalGradient solver I assumed you were thinking something along these lines

J = ||d-Gx||_2^2 + TV(x)

where f(x)=||d-Gx||_2^2 and g(x)=TV(x). I remember thinking about this in the past but getting into troubles because TV(x) has a prox that goes from R^2n -> R^2n whilst the grad of f is R^n -> R^n and so the two cannot 'talk' to each other.... or maybe I am thinking wrongly?

I just want to find an example to show where this new TV operator is useful to add as part of this PR.

[olivierleblanc]

• I added the TV operator and its proximal operator, inspired by the code in PyUnLocBox.

  Makes sense. I guess the idea for this one is to use it with solvers of the form f(x)+g(x), right? As in solvers like PD of the form f(x)+g(Kx) we could already do TV as g=L_2,1 and K=Gradient.
  >> Yes exactly, with this implementation I'm able to use ProximalGradient and GeneralizedProximalGradient!

[...]

• I think I added a relevant test for the 1D case as well as associated docstring. I'm not adding a tutorial using these new features for available time issues, but I can certainly guide someone interested into doing so.

  Sounds good. It would be nice to take one tutorial where we use isotropic TV via Grad+L_21 with PD (eg https://pyproximal.readthedocs.io/en/latest/tutorials/denoising.html#sphx-glr-tutorials-denoising-py) and show how we can use this new operator with for example ProximalGradient. I can give it a go and ping you when I have something.
  >> Perfect. Let us do it like that and don't hesitate to ask anything related.

[olivierleblanc]

@reivilo3 I am going through your new TV operator and I am struggling a bit to understand how that could be used with any of our solvers. Since you modified the ProximalGradient solver I assumed you were thinking something along these lines

J = ||d-Gx||_2^2 + TV(x)

where f(x)=||d-Gx||_2^2 and g(x)=TV(x). I remember thinking about this in the past but getting into troubles because TV(x) has a prox that goes from R^2n -> R^2n whilst the grad of f is R^n -> R^n and so the two cannot 'talk' to each other.... or maybe I am thinking wrongly?

I just want to find an example to show where this new TV operator is useful to add as part of this PR.

I made a new commit with a tutorial like Python Notebook which uses ProximalGradient with this TV operator.
The Gradient is in R^{2N} but the prox is in R^N. See equation (4.6) and associated algorithms in https://www.tau.ac.il/~becka/27.pdf.

[mrava87]

Thanks! I see now :)

I think I got confused as the way all proximal operators are implemented is slightly different from your TV. Basically to allow easy interoperability with pylops we always force the input output of a proximal to become a vector and internally we do the needed reshaping (I will adapt this to be consistent). But now it’s clear, also for the 2d case the input and output of your TV will be 2d array with same dimensions :)

Regarding the notebook, can I move it to https://github.com/PyLops/pylops_notebooks/tree/master/pyproximal or even better you could make a PR. We don’t want to have notebooks in the main library repo as they don’t play well with version control and add actually quite a lot in terms of size to the repo.

In coming days, I will take some parts of your notebook and included in an example of the documentation like all the others we have and do a bit of clean up of the docstring - I see some parameters are not used whilst still present in the docstring. Then we can merge this PR :)

[olivierleblanc]

Regarding the notebook, can I move it to https://github.com/PyLops/pylops_notebooks/tree/master/pyproximal or even better you could make a PR. We don’t want to have notebooks in the main library repo as they don’t play well with version control and add actually quite a lot in terms of size to the repo.

Sure you can. Note actually they do on VS Code, the application I'm using to code. I suggest you to have a try.

In coming days, I will take some parts of your notebook and included in an example of the documentation like all the others we have and do a bit of clean up of the docstring - I see some parameters are not used whilst still present in the docstring. Then we can merge this PR :)

Perfect!

[mrava87]

Great :) I have some experience with VS code and notebooks, and I agree it is much better in terms of integration with git. However for code packages we prefer to keep notebooks out and have them in separate repositories.

I finalized your PR with some minor cleaning of the code and more importantly I introduced a change such that the input and output of prox is always an array like for every other operator (which is needed to work nicely with solvers when dealing with multiple dimensions). I wanted to look if we could reduce code by using the adjoint of Gradient instead of making it by hand (but for now it works so I will leave it).

Finally, you left a todo (# TODO implement test_gamma), what do you mean?

[olivierleblanc]

Thank you for the quick merge!

Finally, you left a todo (# TODO implement test_gamma), what do you mean?

I copied this comment from PyUnLocBox so it is not mine. I guess it may be an inequality condition where \gamma should not be too large in order for the iterations to converge. You are free to remove this comment if you will.