Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Regression from ECOS causing CVXPY test failures #120

Open
PTNobel opened this issue Jun 23, 2024 · 3 comments · May be fixed by #136
Open

Regression from ECOS causing CVXPY test failures #120

PTNobel opened this issue Jun 23, 2024 · 3 comments · May be fixed by #136

Comments

@PTNobel
Copy link

PTNobel commented Jun 23, 2024

We were investigating some of the test case failures as we migrate the CVXPY test suite from ECOS to Clarabel in cvxpy/cvxpy#2479

It seems at least some of the cases are related to when ECOS correctly reports PRIMAL INFEASIBLE, but Clarabel reports InsufficientProgress instead.

This breaks our disciplined quasiconvex programing work and is consistent with issues that the EE 364a course staff had with Clarabel when assigning bisection methods in the homework.

Frankly, I'm a little worried about what to do here; the 1.5.x CVXPY release apparently regresses the DQCP support compared to prior releases. I'm not sure if this is just an issue of different default feastols or if this is from a new infeasibility detection approach.

Here's a minimized example:

import scipy.sparse as sp
import numpy as np

def make_array(d):
    out = sp.dok_matrix((8, 3))
    for (i, j), v in d.items():
        out[i, j] = v
    return out.tocsc()

G = make_array({
    (0, 0): -1.0,
    (2, 0): 1.0,
    (4, 0): -1.0,
    (1, 1): -1.0,
    (3, 1): 1.0,
    (5, 1): -1.0,
    (6, 1): -1.0,
    (4, 2): 72.001953125,
    (5, 2): -1.0,
    (6, 2): 1.0})
h = np.array([ 0.,  0., 12.,  6.,  0.,  0.,  0.,  2.])
c = np.array([0., 0., 0.])
nonneg = 5
soc = [3]

import ecos

solution = ecos.solve(c, G, h, {'l': 5, 'q': [3]})


import clarabel

P = sp.dok_matrix((3, 3)).tocsc()

cones = [clarabel.NonnegativeConeT(5), clarabel.SecondOrderConeT(3)]
settings = clarabel.DefaultSettings()
solver = clarabel.DefaultSolver(P,c,G,h,cones,settings)
solution = solver.solve()
@PTNobel PTNobel mentioned this issue Jun 23, 2024
Merged
9 tasks
@mipals
Copy link

mipals commented Jun 26, 2024

Having looked the code I do not think there is anything to worry about. It seems as the old infeasibility checks are still present. Adding the ones from the preprint seem to fix the issue (see here).

The only thing that is not really described in the preprint is a multiplication with 1000, which Paul also mentions in a comment in the code should be fixed

Clarabel.rs/src/solver/implementations/default/info.rs

Line 317 in d1ca25d

//PJG hardcoded factor 1000 here should be fixed

I am not sure what fix Paul was thinking of, but I simply removed the multiplication.

@mipals mipals mentioned this issue Jun 27, 2024
Open
@mipals
Copy link

mipals commented Jun 28, 2024

Having looked at this again, I can see that I missed the scalings that happens in the updating of info. As such the above is not really a fix. However, I did notice that a normx is missing here

Clarabel.rs/src/solver/implementations/default/info.rs

Line 115 in d1ca25d

self.res_primal_inf = residuals.rx_inf.norm_scaled(dinv) / T::max(T::one(), normz);

For the simple problem here, it does seem like the big reason that it fail is due to the multiplication of 1000, as the ktratio never gets above 1e9 before InsufficientProgress is reported.

@PTNobel PTNobel mentioned this issue Aug 3, 2024
Closed
@yuwenchen95
Copy link
Collaborator

It seems that we shouldn't increase the infeasibility tolerance reduced_tol_infeas_abs from 1e-8 to 5e-5 for reduced accuracy check. In a feasibility check, increasing tol_feas_abs will relax the checking condition but the relaxed infeasibility check should be the opposite way, i.e. decreasing tol_infeas_abs to allow more relaxed negativity check in this line.

Another issue here is the product of \kappa*\tau becomes lower than the machine precision eps(T), which is another potential numerical issue.

I guess we should fix it in the new version @goulart-paul.

@goulart-paul goulart-paul linked a pull request Nov 25, 2024 that will close this issue
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Port of julia #179 oxfordcontrol/Clarabel.rs
3 participants
@mipals @PTNobel @yuwenchen95