osqp / osqp-matlab

Matlab interface for OSQP
https://osqp.org/
Apache License 2.0
42 stars 25 forks source link

Solver codegen generates incorrect csc matrix for linsys_solver_L #20

Closed abrandemuehl closed 3 years ago

abrandemuehl commented 4 years ago

On the master branch the workspace.c file generated seems to set the linsys_solver_L nzmax variable to a random value at each codegen. My guess would be uninitialized memory usage but I haven't tracked down where it's coming from yet.

clc; clear all; close all;

% Load P, q, A, l, u.
P = zeros(500, 500);
A = zeros(500, 500);
q = zeros(1, 500);
l = zeros(500, 1);
u = zeros(500, 1);

problem = osqp;
settings = problem.default_settings();
settings.verbose = 1;
problem.setup(P, q, A, l, u, settings);

% Not using LONG means that the compiler should warn you that you're overflowing an integer
% if you get a very big random value
problem.codegen('osqp_solver', 'force_rewrite', true, 'FLOAT', true, 'LONG', false);

problem.solve()
goulart-paul commented 4 years ago

When I do this I get nzmax initialised to -1, but this may be a difference of versions. Could you please post the version number that you are using?

Note that since your problem data is entirely zeros, you should expect that the LDL factorisation of the KKT matrix to result in an empty L matrix. The D matrix will have terms determined by the values of sigma and rho. I'm not sure if nzmax = -1 for L is correct either in this case btw, so that will require some follow up.

abrandemuehl commented 4 years ago

I'm using fa7362e9fbd2140e89a2979d7a1bf91c3c07b4f8 (current master) and if I revert to tag v0.6.0 it still occurs.

I chose zeros just to make it easy to create.

// The line that is generated in workspace.c
csc linsys_solver_L = {30681249209647152, 740, 740, linsys_solver_L_p, linsys_solver_L_i, linsys_solver_L_x, -1};
imciner2 commented 4 years ago

@goulart-paul the only -1 I get is for the nz field of the csc struct. The nzmax field (the first one) does appear to be uninitialized in the solver. It looks like it should be set inside LDL_factor (qdldl_interface.c:71) when the actual matrix is allocated, but it isn't. It looks like the best thing to do is to just set the nzmax field to be the number of actual non-zeros in the L matrix. I can put a PR together for this if you agree.

goulart-paul commented 4 years ago

You are correct and I agree. I wonder if that value is ever actually used anywhere in our code, since we don't ever reallocate arrays within a csc formatted matrix. I'm not sure what it's for.

goulart-paul commented 4 years ago

I think this is fixed by Ian's PR. Please let us know if it continues to be an issue.

abrandemuehl commented 4 years ago

Looks like it's fixed! I'll leave it up to you whether you want to close this now or wait for the submodule to be updated in this repository