astro-informatics / purify

Next-generation radio interferometric imaging.
https://astro-informatics.github.io/purify
GNU General Public License v2.0
16 stars 11 forks source link

admm.cc example not converging #29

Closed Luke-Pratley closed 8 years ago

Luke-Pratley commented 8 years ago

It looks like nothing is changing with iterations, and various values are always zero.

Luke-Pratley commented 8 years ago

@mdavezac, the change you made in bf9a2cc does stop ADMM from breaking, but I still feel something is wrong.

For SDMM, the initial_estimate is an image in vector form, so should have the size of the image. But, your fix for ADMM suggests the initial_estimate is the size of a visibility vector, so it is not an image.

mdavezac commented 8 years ago

Okay, I'll look into it.

mdavezac commented 8 years ago

After way too much time, I know remember it was a design decision :) Possibly a bad one.

When designing an algorithm we would to do the setup once and the optimization N times over different inputs. And, yes it should probably be consistent between algorithms :blush:.

So what's the best input? an initial guess, or the visibilities?

@Luke-Pratley, @jasonmcewen, which should designated an input? and which a problem setup?

rafael-carrillo commented 8 years ago

Hello all,

PADMM is defined as min_{x, z} f(x) + g(z) with Phi x + z = y. So now the visibilities become part of the definition of the problem. So we can have two possible input vectors: an initial guess x and/or the visibilities y.

Actually, the initial guess is for the slack variable z that represents the residual, i.e z = Phi x - y, thus of the visibility vector size. For PADMM is more important to have an initial guess for x, i.e the image. An initial guess for z is not really needed since already in the first iteration the algo does an update.

However for warm start purposes it’s more important to have an initial guess of the Lagrange multiplier vector. For example if you are going to run a reweighed algorithm you would need to pass an initial solution, x, and an initial vector of Lagrange multipliers such that the algorithm starts in the good point.

I hope this is of help.

On 18 Apr 2016, at 14:03, mdavezac notifications@github.com wrote:

After way too much time, I know remember it was a design decision :) Possibly a bad one.

SDMM is defined as min_x f(L_f x) + g(L_g x), with the visibilities folded into the definition of f (or g). So the only input in this formulation would be the initial guess. Nominally, any guess should do, so it is possible to define SDMM not to take any input vector either.

PADMM is defined as min_{x, z} f(x) + g(z) with Phi x + z = y. So now the visibilities become part of the definition of the problem. So we can have two possible input vectors: an initial guess x and/or the visibilities y.

When designing an algorithm we would to do the setup once and the optimization N times over different inputs. And, yes it should probably be consistent between algorithms .

So what's the best input? an initial guess, or the visibilities?

@Luke-Pratley https://github.com/Luke-Pratley, @jasonmcewen https://github.com/jasonmcewen, which should designated an input? and which a problem setup?

— You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub https://github.com/astro-informatics/purify/issues/29#issuecomment-211350698

Luke-Pratley commented 8 years ago

@mdavezac @jasonmcewen, to me it makes sense to have an image, x, as the initial guess. It sounds like this is consistent with @rafael-carrillo thinks. I don't know about problem setup though.

mdavezac commented 8 years ago

Okay, I'll start refactoring so the input is the initial guess

mdavezac commented 8 years ago

@rafael-carrillo, in the C version of padmm the initial values in xsol are never used. In all cases, xsol gets overwritten when computing sopt_prox_l1, before it is ever used. Did read that right?

rafael-carrillo commented 8 years ago

@mdavezac This is strange because the first time we update xsol it should be in two steps:

xsol = xsol - mu*gradient

sol = prox_L1(xsol).

Can you point me to the first line that overwrites xsol just to check?

On 18 Apr 2016, at 15:48, mdavezac notifications@github.com wrote:

@rafael-carrillo https://github.com/rafael-carrillo, in the C version of padmm the initial values in xsol are never used. In all cases, xsol gets overwritten when computing sopt_prox_l1, before it is ever used. Did read that right?

— You are receiving this because you were mentioned. Reply to this email directly or view it on GitHub https://github.com/astro-informatics/purify/issues/29#issuecomment-211388100

mdavezac commented 8 years ago

In a special case, it is first overwritten (and used) here.

In the general case, it is fist overwritten and used when computing the l1 proximal

Within sopt_l1_prox, it if first used and overwritten here or here depending on whether it is real or complex.

rafael-carrillo commented 8 years ago

This is a mistake in the code. Basically we are always initialising xsol as At(y)/nu.

We should have the option to receive xsol and z as inputs. In the code right now xsol is initialised to At(y)/nu and z is always initialised to the zero vector.

We never took that into account but for the reweighed scheme we need such warm starts.

On 18 Apr 2016, at 16:27, mdavezac notifications@github.com wrote:

In a special case, it is first overwritten (and used) here https://github.com/astro-informatics/sopt/blob/development/src/c/sopt_l1.c#L1900.

In the general case, it is fist overwritten and used when computing the l1 proximal https://github.com/astro-informatics/sopt/blob/development/src/c/sopt_l1.c#L2013 Within sopt_l1_prox, it if first used and overwritten here https://github.com/astro-informatics/sopt/blob/development/src/c/sopt_prox.c#L95 or here https://github.com/astro-informatics/sopt/blob/development/src/c/sopt_prox.c#L112 depending on whether it is real or complex.

— You are receiving this because you were mentioned. Reply to this email directly or view it on GitHub https://github.com/astro-informatics/purify/issues/29#issuecomment-211403496