search

jolars / slopecd

4 stars 2 forks source link

sign beta_tilde #11

Closed JonasWallin closed 2 years ago

JonasWallin commented 2 years ago

https://github.com/QB3/slopecd/blob/9c1a2e80da8fba48ee8da3e8f08a79f1243075dd/code/slope/solvers/hybrid.py#L23

I don't think this correct, I think one should not use abs here. It will probably matter very little. However, if beta_tilde changes sign then all betas should change sign.

Klopfe commented 2 years ago

yes, you are right!

jolars commented 2 years ago

Yes, but isn't that something we should allow? I don't think it causes any trouble.

JonasWallin commented 2 years ago

I just meant that we should remove abs there.

Klopfe commented 2 years ago

I can do that and push if everyone's ok

jolars commented 2 years ago

Hm, okay, but do we actually use the signs of c anywhere? In the update, we are already updating the signs of beta properly I think. But, yeah, maybe you're right, since keeping track of the signs would allow us to not have to update beta at all.

jolars commented 2 years ago

Just be careful because I think the code may occasionally rely on c being the absolute values of the coefficients.

Klopfe commented 2 years ago

yes I see, it broke the proxsplit solver, sorry I didnt think of that

jolars commented 2 years ago

That may actually not be your fault :) I'm not sure the version here is working correctly. Go ahead and make the change, I'll take care of updating the proxsplit solver later on.