Chambolle-Pock

[olivierleblanc]

I added Chambolle-Pock primal-dual algorithm. I also added a parameter in the 'norm_nuclear' function to benefit from the hermitian property in 'np.linalg.svd'.

[nperraud]

@reivilo3 thank you for your contribution
@mdeff do you know why the tests are not starting?

[olivierleblanc]

I didn't notice I added a while ago some tests on an extended version of ADMM called linearized ADMM and inspired from "Proximal Algorithms. N. Parikh and S. Boyd. Foundations and Trends in Optimization, 1(3):123-231, 2014."
This extension has an issue because:
for an input with size N1 and an output with size N2, thus transformation matrix N2xN1, the prox with L1-norm returns a signal
of size N1 instead of N2, which causes dimension incompatibilities in the algorithm.

I didn't manage to fix this issue. However I still let this not working implementation as it doesn't affect the default version of ADMM.

I commented the test on linearized-ADMM in test_solvers.py

[nperraud]

Hi reivilo3,

This should not be a problem I believe. Looking at the algorithm, the AˆT should returns things in the right dimension.

I have started test with different dimensions, I believe they are crucial to avoid mistakes...

I looked at the code quickly and couldn't figure out the problem either though...

[nperraud]

A question of nomenclature. Linearized ADMM is a generalization of ADMM or Douglas Rachford?
I am not sure...

[nperraud]

Reference document: https://web.stanford.edu/~boyd/papers/pdf/prox_algs.pdf

[olivierleblanc]

Yes for the first line, but for the second, this returns z^{k+1} with dimension N1, which is not compatible with u^k and Ax^{K+1} being of dimensions N2. Do you agree?

About your second message, according to p153 of the reference document, ADMM and Douglas-Rashford are the equivalent.

[nperraud]

About your second message, according to p153 of the reference document, ADMM and Douglas-Rashford are the equivalent.

Thanks... We should probably make a note in the documentation for that.

Yes for the first line, but for the second, this returns z^{k+1} with dimension N1, which is not compatible with u^k and Ax^{K+1} being of dimensions N2. Do you agree?

I think you should do the prox of $g(\cdot)$, not the prox of $g(A \cdot)$. This will return z of shape N2. Am I missing something?

[olivierleblanc]

If I do the prox of $g(\cdot)$, there are some calls to _eval and _objective which take the initialization x0 for the solver as input, with dimension N1, and it typically throws an error at :
sol = self.A(x) - self.y()
because self.A is None, x is of size N1 and y of size N2

[nperraud]

You are correct. This is somehow a design issue of the pyunlocbox.
But, we had the same problem for many other solvers. The way to fix it is to redefine the objective function of the solver.

You can see here how it is done for the primal_dual solver.

pyunlocbox/pyunlocbox/solvers.py

Line 849 in 4a13fea

def _objective(self, x):

In your case, it is probably something like this:

    def _objective(self, x):
        obj_nonsmooth = [self.non_smooth_funs[0].eval(x),
                         self.non_smooth_funs[1].eval(self.L(x))]
        return obj_nonsmooth

[olivierleblanc]

Thank you!
I fixed it.

Rewriting the algorithm versions:
Default:
x^{k+1} = prox_{λf} (z^k)
z^{k+1} = z^k + prox_{λg}(2x^{k+1}−z^k) − x^{k+1}
Or equivalently:
z^{k+1} = prox_{λg} (x^k+u^k)
x^{k+1} = prox_{λf} (z^{k+1}−u^k)
u^{k+1} = u^k+x^{k+1}−z^{k+1}
If linearized:
z^{k+1} = prox_{λg} (Ax^k+u^k)
x^{k+1} = prox_{µf} (x^k−(µ/λ)A^T(Ax^k−z^{k+1}+u^k))
u^{k+1} = u^k+Ax^{k+1}−z^{k+1}

I felt free to write the same three lines for the algorithm, where self.A(x) is either Ax or identity. I thus removed the parameter "lambda_" in the function "init_" of Douglas-Rashford its job is ensured by "step". Calling the "step" parameter θ, I noticed that the current version was written as:
z^{k+1} = z^k + λ [ prox_{θg}(2x^k−z^k) − x^k ]
x^{k+1} = prox_{θf} (z^{k+1})
instead of:
z^{k+1} = z^k + prox_{θg}(2x^k−z^k) − x^k
x^{k+1} = prox_{θf} (z^{k+1})

[nperraud]

@reivilo3 Thank you very much. For now I need to get the test starting again before possibly doing a merge.

I am not sure what happened for the lambda. I need to check the original paper to see if we made a mistake or if there was a reason for it.

[mdeff]

@mdeff do you know why the tests are not starting?

Travis changed (again) how things work. I had to enable a plan at the organization level... It should work now.

[mdeff]

Many thanks for your contributions @reivilo3! Here's some feedback. I'm leaving @nperraud checking the correctness of the algorithms.

[mdeff]

@reivilo3, can you check the "Allow edits from maintainers" box? Also, the tests should pass. Thanks!

[olivierleblanc]

@mdeff The box was already checked!

[mdeff]

Oh you're right. I could push. :) I made a mistake when trying it at first...

[olivierleblanc]

@reivilo3 Thank you very much. For now I need to get the test starting again before possibly doing a merge.

I am not sure what happened for the lambda. I need to check the original paper to see if we made a mistake or if there was a reason for it.

Here is another source for the algorithm, see p482 : https://www2.karlin.mff.cuni.cz/~vybiral/Sem-2017/Rauhut.pdf
This may suggest to write :

        self.z[:] = self.non_smooth_funs[1].prox(self.A(self.sol) + self.lambda_*self.u, self.lambda_)
        self.sol[:] = self.non_smooth_funs[0].prox(self.sol-self.mu*self.At(self.A(self.sol)-self.z+self.u), self.mu)
        self.u[:] = self.u + self.step*(self.A(self.sol) - self.z)

And thus reinsert the lambda parameter
But in this version the prox_star is used for the first function F. There could be subtleties..

[mdeff]

Thanks for addressing my feedback! While you reach a conclusion with @nperraud about the lambda parameter, could we make Travis happy? You can run the checks locally with make lint, make test, and make doc.

[olivierleblanc]

I never used 'make' on my pc with Windows 10, so downloaded Chocolatey and got some errors which are maybe common.

• make lint:
  is it possible to remove this limit of 79 characters a line? And about the other errors, do I need to correct them at hand?

• make doc:
  

• make test:
  

I don't know how to deal with these issues thus need your help.

[nperraud]

For me the algorithm look correct and the test case added seems to cover most of it. So I will give it a pass.

I agree that it is better remove the lambda_ and use step instead in Douglas-Rachford. I do not believe we loose something and the interface is simpler for the uer.

The make lint works... For the two others, you probably need to use a linux console to run them. On windows, you could install Windows Subsytem for Linux... Alternatively, you could use the test in Travis:
https://app.travis-ci.com/github/epfl-lts2/pyunlocbox/jobs/526207158

We would like to keep the 79 character limits because it is a good practice for code. For all the stuff related to make lint, you might be able to install some extension to you editor that could help you (search for PEP8 or autoPEP8).

[nperraud]

I approve the PR from an algorithmic point of view.

[mdeff]

Please fix the cosmetic issues reported by make lint.

make doc complains about a missing package. The easiest fix is to create a new virtual environment (without access to system package) and install with pip install --upgrade --editable pyunlocbox[dev]. Please check the CONTRIBUTING.rst file.

The make test error is rather cryptic. :/ As @nperraud suggested, I'd encourage you to try the WSL if that's not yet what you're using.

[mdeff]

I fixed some issues.

What's most concerning now is that Douglas-Rachford seems to have been somewhat broken. Some tests are failing and the optimization in two tutorials (compressed sensing and denoising) are diverging (see Travis run). Any idea?

[nperraud]

The problem probably comes from the fact that the lambda parameter was replaced by mu. I would need to speed a bit of time to fix it and I don't have this time now unfortunately.

[mdeff]

Maybe @reivilo3 could fix that?

BTW, we should aim to name parameters meaningfully, i.e., like step and unlike lambda_and mu. I know old code got that wrong, and should be updated sometime (in another PR of course).

We're otherwise not far from being able to merge this. :)

[olivierleblanc]

I tried a couple things to resolve the bugs this morning but didn't success...
The maybe most promising commit is 7fa8b69, and we should do a 5a5e844-like modification to use the previous two steps iterations when there is no linear operator A, as your test require a given number of iterations for some tests.