Skip to content

Commit 4248f7d

Browse files
Transurgeonclaude
andcommitted
Mark param_source subtrees dirty before the owning atom re-evaluates them
A param_source subtree is evaluated by the owning atom's gated recursion, not the main forward walk, so expr_set_needs_refresh never reaches nodes inside a composite source. Gated nodes there (promote, nested mults, cached matmul coefficients) kept serving values cached at build time after problem_update_params: p*A @ x and quad_form(x, g*Sig) both solved with the original parameter values on every re-solve. Fix: each source-owning atom marks its subtree dirty at the moment it is about to re-evaluate it (one expr_set_needs_refresh call per atom, gated on the flag the main walker already sets). Nested gated levels recurse for free. No struct or walker changes; no behavior change for one-shot solves or bare-parameter sources. Tests: tests/problem/test_param_source_refresh.h pins the three failing shapes (composite matmul coefficient, composite quad matrix, two nested gated levels); each fails without the fix and passes with it. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
1 parent 631aece commit 4248f7d

8 files changed

Lines changed: 221 additions & 0 deletions

File tree

‎src/atoms/affine/convolve.c‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -39,6 +39,9 @@ static void forward(expr *node, const double *u)
3939

4040
if (cnode->base.needs_parameter_refresh)
4141
{
42+
/* Composite sources hold gated nodes of their own (promote, nested
43+
mults): mark the whole side subtree before re-evaluating it. */
44+
expr_set_needs_refresh(cnode->param_source);
4245
cnode->param_source->forward(cnode->param_source, NULL);
4346
/* refresh the convolution matrix values if it exists (necessary to check
4447
for null in case someone calls forward before initializing the jacobian,

‎src/atoms/affine/kron.c‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -44,6 +44,9 @@ static void refresh_param_values(kron_expr *knode)
4444
return;
4545
}
4646

47+
/* Composite sources hold gated nodes of their own (promote, nested
48+
mults): mark the whole side subtree before re-evaluating it. */
49+
expr_set_needs_refresh(knode->param_source);
4750
knode->param_source->forward(knode->param_source, NULL);
4851
knode->base.needs_parameter_refresh = false;
4952
}

‎src/atoms/affine/left_matmul.c‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -70,6 +70,9 @@ static void forward(expr *node, const double *u)
7070
/* call forward on param_source if it exists and needs refresh */
7171
if (lnode->param_source != NULL && lnode->base.needs_parameter_refresh)
7272
{
73+
/* Composite sources hold gated nodes of their own (promote, nested
74+
mults): mark the whole side subtree before re-evaluating it. */
75+
expr_set_needs_refresh(lnode->param_source);
7376
lnode->param_source->forward(lnode->param_source, NULL);
7477
}
7578

‎src/atoms/affine/scalar_mult.c‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -35,6 +35,9 @@ static void forward(expr *node, const double *u)
3535
its values) */
3636
if (snode->base.needs_parameter_refresh)
3737
{
38+
/* Composite sources hold gated nodes of their own (promote, nested
39+
mults): mark the whole side subtree before re-evaluating it. */
40+
expr_set_needs_refresh(snode->param_source);
3841
snode->param_source->forward(snode->param_source, NULL);
3942
snode->base.needs_parameter_refresh = false;
4043
}

‎src/atoms/affine/vector_mult.c‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -35,6 +35,9 @@ static void forward(expr *node, const double *u)
3535
its values) */
3636
if (vnode->base.needs_parameter_refresh)
3737
{
38+
/* Composite sources hold gated nodes of their own (promote, nested
39+
mults): mark the whole side subtree before re-evaluating it. */
40+
expr_set_needs_refresh(vnode->param_source);
3841
vnode->param_source->forward(vnode->param_source, NULL);
3942
vnode->base.needs_parameter_refresh = false;
4043
}

‎src/atoms/other/quad_form.c‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -55,6 +55,9 @@ static void forward(expr *node, const double *u)
5555
/* refresh Q from the parameter if needed (no-op on the constant/sparse path) */
5656
if (qnode->param_source != NULL && node->needs_parameter_refresh)
5757
{
58+
/* Composite sources hold gated nodes of their own (promote, nested
59+
mults): mark the whole side subtree before re-evaluating it. */
60+
expr_set_needs_refresh(qnode->param_source);
5861
qnode->param_source->forward(qnode->param_source, NULL);
5962
}
6063
refresh_param_values_qf(qnode);

‎tests/all_tests.c‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -60,6 +60,7 @@
6060
#include "old-code/test_old_permuted_dense.h"
6161
#include "problem/test_param_broadcast.h"
6262
#include "problem/test_param_prob.h"
63+
#include "problem/test_param_source_refresh.h"
6364
#include "problem/test_problem.h"
6465
#include "utils/test_COO_matrix.h"
6566
#include "utils/test_alloc_overflow.h"
@@ -550,6 +551,11 @@ int main(void)
550551
mu_run_test(test_param_scalar_mult_problem_with_constant, tests_run);
551552
mu_run_test(test_param_convolve_problem, tests_run);
552553

554+
printf("\n--- Composite param_source Refresh Tests ---\n");
555+
mu_run_test(test_composite_source_left_matmul, tests_run);
556+
mu_run_test(test_composite_source_quad_form, tests_run);
557+
mu_run_test(test_composite_source_nested_gates, tests_run);
558+
553559
printf("\n--- Parameter + Broadcast Tests ---\n");
554560
mu_run_test(test_constant_broadcast_vector_mult, tests_run);
555561
mu_run_test(test_constant_promote_vector_mult, tests_run);
Lines changed: 197 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,197 @@
1+
#ifndef TEST_PARAM_SOURCE_REFRESH_H
2+
#define TEST_PARAM_SOURCE_REFRESH_H
3+
4+
/* Composite param_source refresh across problem_update_params.
5+
*
6+
* A param_source subtree is evaluated by the owning atom's gated recursion,
7+
* not the main forward walk, so gated nodes *inside* a composite source
8+
* (promote, nested mults, cached matmuls) are only re-evaluated if the owner
9+
* marks the subtree before forwarding it. These tests pin the shapes that
10+
* served stale values before that mark existed:
11+
*
12+
* 1. left_matmul whose coefficient is p (.) A ("(p*A) @ x")
13+
* 2. quad_form whose matrix is g (.) Sigma ("quad_form(x, g*Sig)")
14+
* 3. two gated levels: coefficient (c*p) (.) A ("((2p)*A) @ x")
15+
*
16+
* Bare-parameter sources are covered by test_param_prob.h; these are the
17+
* composite counterparts.
18+
*/
19+
20+
#include <math.h>
21+
#include <stdio.h>
22+
23+
#include "atoms/affine.h"
24+
#include "atoms/non_elementwise_full_dom.h"
25+
#include "expr.h"
26+
#include "minunit.h"
27+
#include "problem.h"
28+
#include "subexpr.h"
29+
#include "test_helpers.h"
30+
31+
/* (p * A) @ x: the elementwise product of a promoted scalar parameter with a
32+
constant matrix feeds left_matmul as its param_source. */
33+
const char *test_composite_source_left_matmul(void)
34+
{
35+
int n = 2;
36+
37+
expr *x = new_variable(2, 1, 0, n);
38+
expr *objective = new_sum(x, -1);
39+
40+
double theta[1] = {2.0};
41+
expr *p = new_parameter(1, 1, 0, n, theta);
42+
/* column-major A = [[1,2],[3,4]] */
43+
double A_vals[4] = {1.0, 3.0, 2.0, 4.0};
44+
expr *A_const = new_parameter(2, 2, PARAM_FIXED, n, A_vals);
45+
expr *coeff = new_vector_mult(new_promote(p, 2, 2), A_const);
46+
47+
expr *constraint = new_left_matmul_dense(coeff, x, 2, 2, NULL);
48+
expr *constraints[1] = {constraint};
49+
problem *prob = new_problem(objective, constraints, 1, false);
50+
51+
expr *param_nodes[1] = {p};
52+
problem_register_params(prob, param_nodes, 1);
53+
problem_init_derivatives(prob);
54+
55+
double x_vals[2] = {1.0, 2.0};
56+
int Ap[3] = {0, 2, 4};
57+
int Ai[4] = {0, 1, 0, 1};
58+
59+
/* p = 2: effective A is 2*A, so A@x = 2*[5,11] */
60+
problem_constraint_forward(prob, x_vals);
61+
problem_jacobian(prob);
62+
double constrs[2] = {10.0, 22.0};
63+
double Ax[4] = {2.0, 4.0, 6.0, 8.0};
64+
mu_assert("initial vals fail",
65+
cmp_double_array(prob->constraint_values, constrs, 2));
66+
mu_assert("initial jac fail", cmp_double_array(prob->jacobian->x, Ax, 4));
67+
mu_assert("rows fail", cmp_int_array(prob->jacobian->p, Ap, 3));
68+
mu_assert("cols fail", cmp_int_array(prob->jacobian->i, Ai, 4));
69+
70+
/* p = 10: the promote inside the source must re-evaluate */
71+
theta[0] = 10.0;
72+
problem_update_params(prob, theta);
73+
problem_constraint_forward(prob, x_vals);
74+
problem_jacobian(prob);
75+
constrs[0] = 50.0;
76+
constrs[1] = 110.0;
77+
Ax[0] = 10.0;
78+
Ax[1] = 20.0;
79+
Ax[2] = 30.0;
80+
Ax[3] = 40.0;
81+
mu_assert("stale constraint values after update",
82+
cmp_double_array(prob->constraint_values, constrs, 2));
83+
mu_assert("stale jacobian values after update",
84+
cmp_double_array(prob->jacobian->x, Ax, 4));
85+
86+
free_problem(prob);
87+
88+
return 0;
89+
}
90+
91+
/* quad_form(x, g * Sigma): the scaled constant matrix feeds quad_form's
92+
cached Q through its param_source. */
93+
const char *test_composite_source_quad_form(void)
94+
{
95+
int n = 2;
96+
97+
expr *x = new_variable(2, 1, 0, n);
98+
99+
double theta[1] = {1.0};
100+
expr *g = new_parameter(1, 1, 0, n, theta);
101+
/* Sigma = diag(2, 3); symmetric, so major order is irrelevant */
102+
double S_vals[4] = {2.0, 0.0, 0.0, 3.0};
103+
expr *S_const = new_parameter(2, 2, PARAM_FIXED, n, S_vals);
104+
expr *Q_src = new_vector_mult(new_promote(g, 2, 2), S_const);
105+
106+
expr *objective = new_quad_form_dense(x, 2, NULL, Q_src);
107+
problem *prob = new_problem(objective, NULL, 0, false);
108+
109+
expr *param_nodes[1] = {g};
110+
problem_register_params(prob, param_nodes, 1);
111+
problem_init_derivatives(prob);
112+
113+
double x_vals[2] = {1.0, 2.0};
114+
115+
/* g = 1: x'(g Sigma)x = 2*1 + 3*4 = 14; gradient 2(g Sigma)x = [4, 12] */
116+
double obj_val = problem_objective_forward(prob, x_vals);
117+
problem_gradient(prob);
118+
double grad[2] = {4.0, 12.0};
119+
mu_assert("initial obj fail", fabs(obj_val - 14.0) < 1e-10);
120+
mu_assert("initial grad fail", cmp_double_array(prob->gradient_values, grad, 2));
121+
122+
/* g = 5: Q's cached values must be re-copied from the re-evaluated source */
123+
theta[0] = 5.0;
124+
problem_update_params(prob, theta);
125+
obj_val = problem_objective_forward(prob, x_vals);
126+
problem_gradient(prob);
127+
grad[0] = 20.0;
128+
grad[1] = 60.0;
129+
mu_assert("stale objective after update", fabs(obj_val - 70.0) < 1e-10);
130+
mu_assert("stale gradient after update",
131+
cmp_double_array(prob->gradient_values, grad, 2));
132+
133+
free_problem(prob);
134+
135+
return 0;
136+
}
137+
138+
/* ((c * p) * A) @ x: two gated levels inside the source (scalar_mult under
139+
promote under vector_mult) — the mark must recurse through all of them. */
140+
const char *test_composite_source_nested_gates(void)
141+
{
142+
int n = 2;
143+
144+
expr *x = new_variable(2, 1, 0, n);
145+
expr *objective = new_sum(x, -1);
146+
147+
double theta[1] = {1.0};
148+
expr *p = new_parameter(1, 1, 0, n, theta);
149+
double c_val = 2.0;
150+
expr *c_const = new_parameter(1, 1, PARAM_FIXED, n, &c_val);
151+
expr *scaled = new_scalar_mult(p, c_const); /* = c * p, a gated node */
152+
/* column-major A = [[1,2],[3,4]] */
153+
double A_vals[4] = {1.0, 3.0, 2.0, 4.0};
154+
expr *A_const = new_parameter(2, 2, PARAM_FIXED, n, A_vals);
155+
expr *coeff = new_vector_mult(new_promote(scaled, 2, 2), A_const);
156+
157+
expr *constraint = new_left_matmul_dense(coeff, x, 2, 2, NULL);
158+
expr *constraints[1] = {constraint};
159+
problem *prob = new_problem(objective, constraints, 1, false);
160+
161+
expr *param_nodes[1] = {p};
162+
problem_register_params(prob, param_nodes, 1);
163+
problem_init_derivatives(prob);
164+
165+
double x_vals[2] = {1.0, 2.0};
166+
167+
/* p = 1: effective A is 2*A */
168+
problem_constraint_forward(prob, x_vals);
169+
problem_jacobian(prob);
170+
double constrs[2] = {10.0, 22.0};
171+
double Ax[4] = {2.0, 4.0, 6.0, 8.0};
172+
mu_assert("initial vals fail",
173+
cmp_double_array(prob->constraint_values, constrs, 2));
174+
mu_assert("initial jac fail", cmp_double_array(prob->jacobian->x, Ax, 4));
175+
176+
/* p = 5: effective A is 10*A; both gated levels must re-evaluate */
177+
theta[0] = 5.0;
178+
problem_update_params(prob, theta);
179+
problem_constraint_forward(prob, x_vals);
180+
problem_jacobian(prob);
181+
constrs[0] = 50.0;
182+
constrs[1] = 110.0;
183+
Ax[0] = 10.0;
184+
Ax[1] = 20.0;
185+
Ax[2] = 30.0;
186+
Ax[3] = 40.0;
187+
mu_assert("stale constraint values after update",
188+
cmp_double_array(prob->constraint_values, constrs, 2));
189+
mu_assert("stale jacobian values after update",
190+
cmp_double_array(prob->jacobian->x, Ax, 4));
191+
192+
free_problem(prob);
193+
194+
return 0;
195+
}
196+
197+
#endif /* TEST_PARAM_SOURCE_REFRESH_H */

0 commit comments

Comments
 (0)