Skip to content

Commit 000667f

Browse files
Transurgeonclaude
andcommitted
Simplify diff-engine converter cleanups
- Inline the 1D-array reshape in convert_matmul (was a helper used twice). - Extract _matmul_param_node to dedupe the param-source branching. - Trim over-verbose docstrings and WHAT-not-WHY comments in convert_matmul, convert_expr, convert_reshape, convert_transpose, convert_conv, and test_matmul_param_inside_transpose. No behavior change: same 217 passed / 77 skipped / 0 failed in cvxpy/tests/nlp_tests/ and 17/17 new tests green. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
1 parent eb6ef2f commit 000667f

3 files changed

Lines changed: 29 additions & 58 deletions

File tree

cvxpy/reductions/solvers/nlp_solvers/diff_engine/converters.py

Lines changed: 23 additions & 39 deletions
Original file line numberDiff line numberDiff line change
@@ -30,65 +30,51 @@
3030
from cvxpy.reductions.solvers.nlp_solvers.diff_engine.registry import ATOM_CONVERTERS
3131

3232

33-
def _matmul_normalize_1d(A, side):
34-
"""Reshape a 1D numpy array to 2D for matmul.
33+
def _matmul_param_node(arg, child, param_dict):
34+
"""param_node for the constant side of a matmul.
3535
36-
NumPy matmul treats 1D arrays differently depending on which side:
37-
Left 1D: (k,) → (1, k) — row vector
38-
Right 1D: (k,) → (k, 1) — column vector
39-
2D input is returned unchanged.
36+
Returns the parameter capsule for a bare Parameter, the child capsule
37+
when the constant side contains parameters wrapped in an affine atom
38+
(so update_params keeps them live in the DAG), or None otherwise.
4039
"""
41-
if A.ndim == 1:
42-
return A.reshape(1, -1) if side == 'left' else A.reshape(-1, 1)
43-
return A
40+
if isinstance(arg, cp.Parameter):
41+
return param_dict[arg.id]
42+
if arg.parameters():
43+
return child
44+
return None
4445

4546

4647
def convert_matmul(expr, children, var_dict, n_vars, param_dict):
4748
"""Convert matrix multiplication A @ f(x), f(x) @ A, or X @ Y.
4849
49-
NumPy matmul semantics for 1D arrays:
50-
(n,) @ (m,k) → treat left as (1,n) — normalize_shape already does this
51-
(m,k) @ (n,) → treat right as (n,1) — must reshape from (1,n) storage
52-
(n,) @ (n,) → dot product: (1,n) @ (n,1) → scalar
53-
54-
The C engine only has 2D nodes. 1D expressions are stored as (1,n) by
55-
normalize_shape. All 1D→2D matmul normalization is handled here so that
56-
helper functions always receive properly shaped 2D data.
50+
1D operands are stored as (1, n) in the C engine. Left 1D stays (1, n);
51+
right 1D must be reshaped to (n, 1) for matmul.
5752
"""
5853
left_arg, right_arg = expr.args
5954
left_child, right_child = children
6055

61-
# Right 1D child: C stores as (1, n) but matmul needs (n, 1).
62-
# Do this once, before branching — used by all three branches.
6356
if len(right_arg.shape) <= 1 and right_arg.size > 1:
6457
right_child = _diffengine.make_reshape(right_child, right_arg.size, 1)
6558

6659
if left_arg.is_constant():
67-
A = _matmul_normalize_1d(left_arg.value, 'left')
68-
if isinstance(left_arg, cp.Parameter):
69-
param_node = param_dict[left_arg.id]
70-
elif left_arg.parameters():
71-
param_node = left_child
72-
else:
73-
param_node = None
60+
A = left_arg.value
61+
if A.ndim == 1:
62+
A = A.reshape(1, -1)
63+
param_node = _matmul_param_node(left_arg, left_child, param_dict)
7464
if sparse.issparse(A):
7565
return make_sparse_left_matmul(param_node, right_child, A)
7666
return make_dense_left_matmul(param_node, right_child, A)
7767

78-
elif right_arg.is_constant():
79-
A = _matmul_normalize_1d(right_arg.value, 'right')
80-
if isinstance(right_arg, cp.Parameter):
81-
param_node = param_dict[right_arg.id]
82-
elif right_arg.parameters():
83-
param_node = right_child
84-
else:
85-
param_node = None
68+
if right_arg.is_constant():
69+
A = right_arg.value
70+
if A.ndim == 1:
71+
A = A.reshape(-1, 1)
72+
param_node = _matmul_param_node(right_arg, right_child, param_dict)
8673
if sparse.issparse(A):
8774
return make_sparse_right_matmul(param_node, left_child, A)
8875
return make_dense_right_matmul(param_node, left_child, A)
8976

90-
else:
91-
return _diffengine.make_matmul(left_child, right_child)
77+
return _diffengine.make_matmul(left_child, right_child)
9278

9379
# TODO we should support sparse elementwise multiply at some point.
9480
def convert_multiply(expr, children, var_dict, n_vars, param_dict):
@@ -156,9 +142,7 @@ def convert_expr(expr, var_dict, n_vars, param_dict=None):
156142
d1_Python, d2_Python = normalize_shape(expr.shape)
157143

158144
if d1_C != d1_Python or d2_C != d2_Python:
159-
# 1D Python shapes (n,) normalize to (1, n), but the C engine may
160-
# produce (n, 1) — e.g. matrix @ scalar or transpose of a vector.
161-
# Both represent the same 1D data; reshape to match Python convention.
145+
# 1D shape (n,) normalizes to (1, n) but C may produce (n, 1); reshape.
162146
if len(expr.shape) <= 1 and d1_C * d2_C == d1_Python * d2_Python:
163147
C_expr = _diffengine.make_reshape(C_expr, d1_Python, d2_Python)
164148
else:

cvxpy/reductions/solvers/nlp_solvers/diff_engine/registry.py

Lines changed: 4 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -41,12 +41,7 @@ def convert_vstack(expr, children):
4141

4242

4343
def convert_conv(expr, children):
44-
"""Convert cp.conv / cp.convolve to _diffengine.make_convolve.
45-
46-
Both atoms take args = [constant_kernel, signal] with the kernel
47-
validated as constant. The C node computes full 1D convolution
48-
(length m + n - 1) given a length-m kernel capsule and length-n child.
49-
"""
44+
"""Convert cp.conv / cp.convolve (full 1D convolution)."""
5045
return _diffengine.make_convolve(children[0], children[1])
5146

5247

@@ -128,16 +123,13 @@ def convert_quad_form(expr, children):
128123

129124

130125
def convert_reshape(expr, children):
131-
"""Convert reshape.
126+
"""Convert reshape. C-order via transpose(F-reshape(transpose(x))).
132127
133-
F-order (column-major) uses make_reshape directly.
134-
C-order (row-major) is decomposed as: transpose(reshape(transpose(x), (n, m), F))
135-
since reshape(x, (m, n), C) == transpose(reshape(transpose(x), (n, m), F)).
128+
Identity: reshape(x, (m, n), C) == transpose(reshape(transpose(x), (n, m), F)).
136129
"""
137130
d1, d2 = normalize_shape(expr.shape)
138131
if expr.order == "F":
139132
return _diffengine.make_reshape(children[0], d1, d2)
140-
# C-order: transpose input, F-reshape to swapped dims, transpose output
141133
transposed = _diffengine.make_transpose(children[0])
142134
reshaped = _diffengine.make_reshape(transposed, d2, d1)
143135
return _diffengine.make_transpose(reshaped)
@@ -186,8 +178,7 @@ def convert_prod(expr, children):
186178
return _diffengine.make_prod_axis_one(children[0])
187179

188180
def convert_transpose(expr, children):
189-
# In CVXPY, transposing a 1D vector (n,) is a no-op: (n,).T == (n,).
190-
# The C engine stores 1D as (1, n), so we must not flip it to (n, 1).
181+
# 1D transpose is a numpy no-op; C stores 1D as (1, n), don't flip to (n, 1).
191182
if len(expr.args[0].shape) <= 1:
192183
return children[0]
193184

cvxpy/tests/nlp_tests/test_matmul.py

Lines changed: 2 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -162,12 +162,8 @@ def test_matmul_1d_dot(self):
162162
checker.run_and_assert()
163163

164164
def test_matmul_param_inside_transpose(self):
165-
"""Parameter wrapped in transpose on the left-matmul side.
166-
167-
Ensures convert_matmul's param_source fallback keeps the Parameter
168-
referenced in the DAG so update_params doesn't dereference unregistered
169-
memory. Mutate A.value and re-solve to exercise the param update path.
170-
"""
165+
"""Parameter wrapped in transpose on the left-matmul side; re-solve
166+
after mutating A.value to exercise the update_params path."""
171167
np.random.seed(0)
172168
m, p = 4, 5
173169
A1 = np.random.rand(m, p)

0 commit comments

Comments
 (0)