Skip to content

Commit 0f2288c

Browse files
Transurgeonclaude
andcommitted
Fix column-major parameter ordering in parameterized matmul
CVXPY sends parameter values in Fortran (column-major) order, but the matmul refresh functions assumed row-major/CSR order via raw memcpy. This produced incorrect matrix values for non-symmetric matrices. For sparse matrices, iterate the CSR pattern and index into the column-major source array. For dense matrices, exploit the fact that column-major A is row-major A^T to memcpy directly into AT, then transpose to get A. Also fixes a latent bug where sparse update_values would blindly copy the first nnz values from the full d1*d2 parameter array, which is wrong for matrices with structural zeros. Adds tests for rectangular (3x2) and sparse (3x3 with zeros) cases. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
1 parent d673b62 commit 0f2288c

4 files changed

Lines changed: 190 additions & 26 deletions

File tree

src/atoms/affine/left_matmul.c

Lines changed: 20 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -170,24 +170,39 @@ static void refresh_sparse_left(left_matmul_expr *lnode)
170170
{
171171
Sparse_Matrix *sm_A = (Sparse_Matrix *) lnode->A;
172172
Sparse_Matrix *sm_AT = (Sparse_Matrix *) lnode->AT;
173-
lnode->A->update_values(lnode->A, lnode->param_source->value);
173+
CSR_Matrix *csr = sm_A->csr;
174+
const double *vals = lnode->param_source->value;
175+
int d1 = lnode->param_source->d1;
176+
177+
/* Parameter values are column-major; extract into CSR data order */
178+
for (int row = 0; row < csr->m; row++)
179+
{
180+
for (int k = csr->p[row]; k < csr->p[row + 1]; k++)
181+
{
182+
int col = csr->i[k];
183+
csr->x[k] = vals[col * d1 + row];
184+
}
185+
}
174186
/* Recompute AT values from A */
175187
AT_fill_values(sm_A->csr, sm_AT->csr, lnode->base.work->iwork);
176188
}
177189

178190
static void refresh_dense_left(left_matmul_expr *lnode)
179191
{
180192
Dense_Matrix *dm_A = (Dense_Matrix *) lnode->A;
193+
Dense_Matrix *dm_AT = (Dense_Matrix *) lnode->AT;
181194
int m = dm_A->base.m;
182195
int n = dm_A->base.n;
183-
lnode->A->update_values(lnode->A, lnode->param_source->value);
184-
/* Recompute AT data (transpose of row-major A) */
185-
Dense_Matrix *dm_AT = (Dense_Matrix *) lnode->AT;
196+
const double *vals = lnode->param_source->value;
197+
198+
/* Column-major A is row-major AT; copy directly into AT */
199+
memcpy(dm_AT->x, vals, m * n * sizeof(double));
200+
/* Transpose AT to get row-major A */
186201
for (int i = 0; i < m; i++)
187202
{
188203
for (int j = 0; j < n; j++)
189204
{
190-
dm_AT->x[j * m + i] = dm_A->x[i * n + j];
205+
dm_A->x[i * n + j] = dm_AT->x[j * m + i];
191206
}
192207
}
193208
}

src/atoms/affine/right_matmul.c

Lines changed: 21 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -40,8 +40,19 @@ static void refresh_sparse_right(left_matmul_expr *lnode)
4040
{
4141
Sparse_Matrix *sm_AT_inner = (Sparse_Matrix *) lnode->A;
4242
Sparse_Matrix *sm_A_inner = (Sparse_Matrix *) lnode->AT;
43-
/* lnode->AT holds the original A; update its values from param */
44-
lnode->AT->update_values(lnode->AT, lnode->param_source->value);
43+
CSR_Matrix *csr_A = sm_A_inner->csr;
44+
const double *vals = lnode->param_source->value;
45+
int d1 = lnode->param_source->d1;
46+
47+
/* Parameter values are column-major; extract into CSR data order */
48+
for (int row = 0; row < csr_A->m; row++)
49+
{
50+
for (int k = csr_A->p[row]; k < csr_A->p[row + 1]; k++)
51+
{
52+
int col = csr_A->i[k];
53+
csr_A->x[k] = vals[col * d1 + row];
54+
}
55+
}
4556
/* Recompute A^T (lnode->A) from A (lnode->AT) */
4657
AT_fill_values(sm_A_inner->csr, sm_AT_inner->csr, lnode->base.work->iwork);
4758
}
@@ -50,16 +61,19 @@ static void refresh_dense_right(left_matmul_expr *lnode)
5061
{
5162
Dense_Matrix *dm_AT_inner = (Dense_Matrix *) lnode->A;
5263
Dense_Matrix *dm_A_inner = (Dense_Matrix *) lnode->AT;
53-
int m_orig = dm_A_inner->base.m; /* original A is m x n */
64+
int m_orig = dm_A_inner->base.m;
5465
int n_orig = dm_A_inner->base.n;
55-
/* Update original A (inner's AT) from param values */
56-
lnode->AT->update_values(lnode->AT, lnode->param_source->value);
57-
/* Recompute A^T (inner's A) from A */
66+
const double *vals = lnode->param_source->value;
67+
68+
/* Column-major A is row-major AT; copy directly into inner A (= A^T) */
69+
memcpy(dm_AT_inner->x, vals, m_orig * n_orig * sizeof(double));
70+
/* Transpose to get row-major A (inner's AT = original A) */
5871
for (int i = 0; i < m_orig; i++)
5972
{
6073
for (int j = 0; j < n_orig; j++)
6174
{
62-
dm_AT_inner->x[j * m_orig + i] = dm_A_inner->x[i * n_orig + j];
75+
dm_A_inner->x[i * n_orig + j] =
76+
dm_AT_inner->x[j * m_orig + i];
6377
}
6478
}
6579
}

tests/all_tests.c

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -354,6 +354,8 @@ int main(void)
354354
mu_run_test(test_param_vector_mult_problem, tests_run);
355355
mu_run_test(test_param_left_matmul_problem, tests_run);
356356
mu_run_test(test_param_right_matmul_problem, tests_run);
357+
mu_run_test(test_param_left_matmul_rectangular, tests_run);
358+
mu_run_test(test_param_left_matmul_sparse, tests_run);
357359
mu_run_test(test_param_fixed_skip_in_update, tests_run);
358360
#endif /* PROFILE_ONLY */
359361

tests/problem/test_param_prob.h

Lines changed: 147 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -164,14 +164,14 @@ const char *test_param_vector_mult_problem(void)
164164
* Test 3: left_param_matmul in constraint
165165
*
166166
* Problem: minimize sum(x), subject to A @ x, x size 2, A is 2x2
167-
* A is a 2x2 matrix parameter (param_id=0, size=4, CSR data order)
168-
* A = [[1,2],[3,4]] → CSR data order theta = [1,2,3,4]
167+
* A is a 2x2 matrix parameter (param_id=0, size=4, column-major order)
168+
* A = [[1,2],[3,4]] → column-major theta = [1,3,2,4]
169169
*
170170
* At x=[1,2]:
171171
* constraint_values = [1*1+2*2, 3*1+4*2] = [5, 11]
172172
* jacobian = [[1,2],[3,4]]
173173
*
174-
* After update A = [[5,6],[7,8]] → theta = [5,6,7,8]:
174+
* After update A = [[5,6],[7,8]] → column-major theta = [5,7,6,8]:
175175
* constraint_values = [5*1+6*2, 7*1+8*2] = [17, 23]
176176
* jacobian = [[5,6],[7,8]]
177177
*/
@@ -208,8 +208,8 @@ const char *test_param_left_matmul_problem(void)
208208
problem_register_params(prob, param_nodes, 1);
209209
problem_init_derivatives(prob);
210210

211-
/* Set A = [[1,2],[3,4]], CSR data order: [1,2,3,4] */
212-
double theta[4] = {1.0, 2.0, 3.0, 4.0};
211+
/* Set A = [[1,2],[3,4]], column-major: [1,3,2,4] */
212+
double theta[4] = {1.0, 3.0, 2.0, 4.0};
213213
problem_update_params(prob, theta);
214214

215215
double u[2] = {1.0, 2.0};
@@ -233,8 +233,8 @@ const char *test_param_left_matmul_problem(void)
233233
double expected_x[4] = {1.0, 2.0, 3.0, 4.0};
234234
mu_assert("jac->x wrong (A1)", cmp_double_array(jac->x, expected_x, 4));
235235

236-
/* Update A = [[5,6],[7,8]], CSR data order: [5,6,7,8] */
237-
double theta2[4] = {5.0, 6.0, 7.0, 8.0};
236+
/* Update A = [[5,6],[7,8]], column-major: [5,7,6,8] */
237+
double theta2[4] = {5.0, 7.0, 6.0, 8.0};
238238
problem_update_params(prob, theta2);
239239

240240
problem_constraint_forward(prob, u);
@@ -256,14 +256,14 @@ const char *test_param_left_matmul_problem(void)
256256
* Test 4: right_param_matmul in constraint
257257
*
258258
* Problem: minimize sum(x), subject to x @ A, x size 1x2, A is 2x2
259-
* A is a 2x2 matrix parameter (param_id=0, size=4, CSR data order)
260-
* A = [[1,2],[3,4]] → CSR data order theta = [1,2,3,4]
259+
* A is a 2x2 matrix parameter (param_id=0, size=4, column-major order)
260+
* A = [[1,2],[3,4]] → column-major theta = [1,3,2,4]
261261
*
262262
* At x=[1,2]:
263263
* constraint_values = [1*1+2*3, 1*2+2*4] = [7, 10]
264264
* jacobian = [[1,3],[2,4]] = A^T
265265
*
266-
* After update A = [[5,6],[7,8]] → theta = [5,6,7,8]:
266+
* After update A = [[5,6],[7,8]] → column-major theta = [5,7,6,8]:
267267
* constraint_values = [1*5+2*7, 1*6+2*8] = [19, 22]
268268
* jacobian = [[5,7],[6,8]] = A^T
269269
*/
@@ -300,8 +300,8 @@ const char *test_param_right_matmul_problem(void)
300300
problem_register_params(prob, param_nodes, 1);
301301
problem_init_derivatives(prob);
302302

303-
/* Set A = [[1,2],[3,4]], CSR data order: [1,2,3,4] */
304-
double theta[4] = {1.0, 2.0, 3.0, 4.0};
303+
/* Set A = [[1,2],[3,4]], column-major: [1,3,2,4] */
304+
double theta[4] = {1.0, 3.0, 2.0, 4.0};
305305
problem_update_params(prob, theta);
306306

307307
double u[2] = {1.0, 2.0};
@@ -325,8 +325,8 @@ const char *test_param_right_matmul_problem(void)
325325
double expected_x[4] = {1.0, 3.0, 2.0, 4.0};
326326
mu_assert("jac->x wrong (A1)", cmp_double_array(jac->x, expected_x, 4));
327327

328-
/* Update A = [[5,6],[7,8]], CSR data order: [5,6,7,8] */
329-
double theta2[4] = {5.0, 6.0, 7.0, 8.0};
328+
/* Update A = [[5,6],[7,8]], column-major: [5,7,6,8] */
329+
double theta2[4] = {5.0, 7.0, 6.0, 8.0};
330330
problem_update_params(prob, theta2);
331331

332332
problem_constraint_forward(prob, u);
@@ -425,4 +425,137 @@ const char *test_param_fixed_skip_in_update(void)
425425
return 0;
426426
}
427427

428+
/*
429+
* Test 6: left_param_matmul with rectangular 3x2 matrix
430+
*
431+
* Problem: minimize sum(x), subject to A @ x, x size 2, A is 3x2
432+
* A = [[1,2],[3,4],[5,6]] → column-major theta = [1,3,5,2,4,6]
433+
*
434+
* At x=[1,2]:
435+
* constraint_values = [1+4, 3+8, 5+12] = [5, 11, 17]
436+
* jacobian = [[1,2],[3,4],[5,6]]
437+
*/
438+
const char *test_param_left_matmul_rectangular(void)
439+
{
440+
int n_vars = 2;
441+
442+
/* Objective: sum(x) */
443+
expr *x_obj = new_variable(2, 1, 0, n_vars);
444+
expr *objective = new_sum(x_obj, -1);
445+
446+
/* Constraint: A @ x, A is 3x2 */
447+
expr *x_con = new_variable(2, 1, 0, n_vars);
448+
expr *A_param = new_parameter(3, 2, 0, n_vars, NULL);
449+
450+
/* Dense 3x2 CSR */
451+
CSR_Matrix *A = new_csr_matrix(3, 2, 6);
452+
int Ap[4] = {0, 2, 4, 6};
453+
int Ai[6] = {0, 1, 0, 1, 0, 1};
454+
double Ax[6] = {0, 0, 0, 0, 0, 0};
455+
memcpy(A->p, Ap, 4 * sizeof(int));
456+
memcpy(A->i, Ai, 6 * sizeof(int));
457+
memcpy(A->x, Ax, 6 * sizeof(double));
458+
459+
expr *constraint = new_left_matmul(A_param, x_con, A);
460+
free_csr_matrix(A);
461+
462+
expr *constraints[1] = {constraint};
463+
464+
problem *prob = new_problem(objective, constraints, 1, false);
465+
466+
expr *param_nodes[1] = {A_param};
467+
problem_register_params(prob, param_nodes, 1);
468+
problem_init_derivatives(prob);
469+
470+
/* Set A = [[1,2],[3,4],[5,6]], column-major: [1,3,5,2,4,6] */
471+
double theta[6] = {1.0, 3.0, 5.0, 2.0, 4.0, 6.0};
472+
problem_update_params(prob, theta);
473+
474+
double u[2] = {1.0, 2.0};
475+
problem_constraint_forward(prob, u);
476+
problem_jacobian(prob);
477+
478+
double expected_cv[3] = {5.0, 11.0, 17.0};
479+
mu_assert("rect constraint values wrong",
480+
cmp_double_array(prob->constraint_values, expected_cv, 3));
481+
482+
CSR_Matrix *jac = prob->jacobian;
483+
double expected_x[6] = {1.0, 2.0, 3.0, 4.0, 5.0, 6.0};
484+
mu_assert("rect jac->x wrong", cmp_double_array(jac->x, expected_x, 6));
485+
486+
free_problem(prob);
487+
488+
return 0;
489+
}
490+
491+
/*
492+
* Test 7: left_param_matmul with sparse matrix (structural zeros)
493+
*
494+
* Problem: minimize sum(x), subject to A @ x, x size 3, A is 3x3 sparse
495+
* A = [[1, 0, 2],
496+
* [0, 3, 0],
497+
* [4, 0, 5]]
498+
* CSR: p=[0,2,3,5], i=[0,2, 1, 0,2], nnz=5
499+
* Column-major theta (full 9 values): [1,0,4, 0,3,0, 2,0,5]
500+
*
501+
* At x=[1,2,3]:
502+
* constraint_values = [1+6, 6, 4+15] = [7, 6, 19]
503+
* jacobian = A
504+
*/
505+
const char *test_param_left_matmul_sparse(void)
506+
{
507+
int n_vars = 3;
508+
509+
/* Objective: sum(x) */
510+
expr *x_obj = new_variable(3, 1, 0, n_vars);
511+
expr *objective = new_sum(x_obj, -1);
512+
513+
/* Constraint: A @ x, A is 3x3 sparse */
514+
expr *x_con = new_variable(3, 1, 0, n_vars);
515+
expr *A_param = new_parameter(3, 3, 0, n_vars, NULL);
516+
517+
CSR_Matrix *A = new_csr_matrix(3, 3, 5);
518+
int Ap[4] = {0, 2, 3, 5};
519+
int Ai[5] = {0, 2, 1, 0, 2};
520+
double Ax[5] = {0, 0, 0, 0, 0};
521+
memcpy(A->p, Ap, 4 * sizeof(int));
522+
memcpy(A->i, Ai, 5 * sizeof(int));
523+
memcpy(A->x, Ax, 5 * sizeof(double));
524+
525+
expr *constraint = new_left_matmul(A_param, x_con, A);
526+
free_csr_matrix(A);
527+
528+
expr *constraints[1] = {constraint};
529+
530+
problem *prob = new_problem(objective, constraints, 1, false);
531+
532+
expr *param_nodes[1] = {A_param};
533+
problem_register_params(prob, param_nodes, 1);
534+
problem_init_derivatives(prob);
535+
536+
/* A = [[1,0,2],[0,3,0],[4,0,5]], column-major: [1,0,4, 0,3,0, 2,0,5] */
537+
double theta[9] = {1.0, 0.0, 4.0, 0.0, 3.0, 0.0, 2.0, 0.0, 5.0};
538+
problem_update_params(prob, theta);
539+
540+
double u[3] = {1.0, 2.0, 3.0};
541+
problem_constraint_forward(prob, u);
542+
problem_jacobian(prob);
543+
544+
double expected_cv[3] = {7.0, 6.0, 19.0};
545+
mu_assert("sparse constraint values wrong",
546+
cmp_double_array(prob->constraint_values, expected_cv, 3));
547+
548+
CSR_Matrix *jac = prob->jacobian;
549+
mu_assert("sparse jac nnz wrong", jac->nnz == 5);
550+
551+
/* CSR data order: row0=[1,2], row1=[3], row2=[4,5] */
552+
double expected_x[5] = {1.0, 2.0, 3.0, 4.0, 5.0};
553+
mu_assert("sparse jac->x wrong",
554+
cmp_double_array(jac->x, expected_x, 5));
555+
556+
free_problem(prob);
557+
558+
return 0;
559+
}
560+
428561
#endif /* TEST_PARAM_PROB_H */

0 commit comments

Comments
 (0)