chnages from code review with dance

Author: Transurgeon

CLAUDE.md

@@ -101,9 +101,10 @@ Parameters are treated as affine (not constant) for curvature analysis. `param *
 `Dnlp2Smooth` converts DNLP expressions to smooth forms. Canonicalizers in `dnlp2smooth/canonicalizers/` handle atoms like log, exp, sin, power, geo_mean, etc. Registered in `SMOOTH_CANON_METHODS` dict.
 
 ### Diff engine (`cvxpy/reductions/solvers/nlp_solvers/diff_engine/`)
-- `C_problem.py`: Wraps SparseDiffPy C library for AD (objective, gradient, Jacobian, Hessian)
-- `converters.py`: Maps CVXPY atoms to C diff engine nodes via `convert_expr()` (40+ atoms supported)
-- `ConvertContext`: Manages variable/parameter node dictionaries during conversion
+- `c_problem.py`: Wraps SparseDiffPy C library for AD (objective, gradient, Jacobian, Hessian)
+- `converters.py`: Entry point — `convert_expr(expr, var_dict, n_vars, param_dict=None)` recursively converts CVXPY expressions to C diff engine nodes
+- `registry.py`: `ATOM_CONVERTERS` dict mapping atom names to converter functions (40+ atoms)
+- `helpers.py`: Shared utilities (`build_var_dict`, `build_param_dict`, matmul helpers, `normalize_shape`)
 
 ### Solver interfaces (`cvxpy/reductions/solvers/nlp_solvers/`)
 - `nlp_solver.py`: Base `NLPsolver` class with `Bounds` (constraint extraction) and `Oracles` (diff engine wrapper)
@@ -124,7 +125,7 @@ Register in `canonicalizers/__init__.py` → `CANON_METHODS[my_atom] = my_atom_c
 ### 3. Create smooth canonicalizer in `cvxpy/reductions/dnlp2smooth/canonicalizers/` (if atom is smooth)
 Register in `SMOOTH_CANON_METHODS`.
 
-### 4. Add converter in `cvxpy/reductions/solvers/nlp_solvers/diff_engine/converters.py` (for NLP support)
+### 4. Add converter in `cvxpy/reductions/solvers/nlp_solvers/diff_engine/registry.py` → `ATOM_CONVERTERS` (for NLP support)
 
 ### 5. Export in `cvxpy/atoms/__init__.py`
 

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

@@ -37,16 +37,20 @@ def _convert_matmul(expr, children, var_dict, n_vars, param_dict):
 
     if left_arg.is_constant():
         A = left_arg.value
-        param_node = (convert_expr(left_arg, var_dict, n_vars, param_dict)
-                      if param_dict else None)
+        if isinstance(left_arg, Parameter):
+            param_node = param_dict[left_arg.id]
+        else:
+            param_node = None
         if sparse.issparse(A):
             return _make_sparse_left_matmul(param_node, children[1], A)
         return _make_dense_left_matmul(param_node, children[1], A)
 
     elif right_arg.is_constant():
         A = right_arg.value
-        param_node = (convert_expr(right_arg, var_dict, n_vars, param_dict)
-                      if param_dict else None)
+        if isinstance(right_arg, Parameter):
+            param_node = param_dict[right_arg.id]
+        else:
+            param_node = None
         if sparse.issparse(A):
             return _make_sparse_right_matmul(param_node, children[0], A)
         return _make_dense_right_matmul(param_node, children[0], A)
@@ -60,44 +64,18 @@ def _convert_multiply(expr, children, var_dict, n_vars, param_dict):
     left_arg, right_arg = expr.args
 
     if left_arg.is_constant():
-        if param_dict and left_arg.parameters():
-            param_node = convert_expr(left_arg, var_dict, n_vars, param_dict)
-            if left_arg.size == 1:
-                return _diffengine.make_param_scalar_mult(
-                    param_node, children[1])
-            return _diffengine.make_param_vector_mult(
-                param_node, children[1])
-
-        a = _to_dense_float(left_arg.value)
-        if a.size == 1:
-            scalar = float(a.flat[0])
-            if scalar == 1.0:
-                return children[1]
-            return _diffengine.make_const_scalar_mult(children[1], scalar)
-        return _diffengine.make_const_vector_mult(
-            children[1], a.flatten(order='F'))
-
+        if left_arg.size == 1:
+            return _diffengine.make_param_scalar_mult(children[0], children[1])
+        else:
+            return _diffengine.make_param_vector_mult(children[0], children[1])
     elif right_arg.is_constant():
-        if param_dict and right_arg.parameters():
-            param_node = convert_expr(right_arg, var_dict, n_vars, param_dict)
-            if right_arg.size == 1:
-                return _diffengine.make_param_scalar_mult(
-                    param_node, children[0])
-            return _diffengine.make_param_vector_mult(
-                param_node, children[0])
-
-        a = _to_dense_float(right_arg.value)
-        if a.size == 1:
-            scalar = float(a.flat[0])
-            if scalar == 1.0:
-                return children[0]
-            return _diffengine.make_const_scalar_mult(children[0], scalar)
-        return _diffengine.make_const_vector_mult(
-            children[0], a.flatten(order='F'))
-
-    # Neither is constant, use general multiply
-    return _diffengine.make_multiply(children[0], children[1])
-
+        # TODO is this correct? Do we need to swap the arguments?
+        if right_arg.size == 1:
+            return _diffengine.make_param_scalar_mult(children[0], children[1])
+        else:
+            return _diffengine.make_param_vector_mult(children[0], children[1])
+    else:
+        return _diffengine.make_multiply(children[0], children[1])
 
 
 def convert_expr(expr, var_dict, n_vars, param_dict=None):
@@ -121,7 +99,7 @@ def convert_expr(expr, var_dict, n_vars, param_dict=None):
     if isinstance(expr, cp.Constant):
         c = _to_dense_float(expr.value)
         d1, d2 = normalize_shape(expr.shape)
-        return _diffengine.make_constant(d1, d2, n_vars, c.flatten(order='F'))
+        return _diffengine.make_parameter(d1, d2, -1, n_vars, c.flatten(order='F'))
 
     # Recursive case: atoms
     atom_name = type(expr).__name__

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

@@ -45,8 +45,8 @@ def _chain_add(children):
 def _make_sparse_left_matmul(param_node, child, A):
     if not isinstance(A, sparse.csr_matrix):
         A = sparse.csr_matrix(A)
-    return _diffengine.make_sparse_left_matmul(
-        param_node, child,
+    return _diffengine.make_left_matmul(
+        param_node, child, 'sparse',
         A.data.astype(np.float64, copy=False),
         A.indices.astype(np.int32, copy=False),
         A.indptr.astype(np.int32, copy=False),
@@ -55,15 +55,15 @@ def _make_sparse_left_matmul(param_node, child, A):
 
 def _make_dense_left_matmul(param_node, child, A):
     m, n = normalize_shape(A.shape)
-    return _diffengine.make_dense_left_matmul(
-        param_node, child, A.flatten(order='C'), m, n)
+    return _diffengine.make_left_matmul(
+        param_node, child, 'dense', A.flatten(order='C'), m, n)
 
 
 def _make_sparse_right_matmul(param_node, child, A):
     if not isinstance(A, sparse.csr_matrix):
         A = sparse.csr_matrix(A)
-    return _diffengine.make_sparse_right_matmul(
-        param_node, child,
+    return _diffengine.make_right_matmul(
+        param_node, child, 'sparse',
         A.data.astype(np.float64, copy=False),
         A.indices.astype(np.int32, copy=False),
         A.indptr.astype(np.int32, copy=False),
@@ -72,8 +72,8 @@ def _make_sparse_right_matmul(param_node, child, A):
 
 def _make_dense_right_matmul(param_node, child, A):
     m, n = normalize_shape(A.shape)
-    return _diffengine.make_dense_right_matmul(
-        param_node, child, A.flatten(order='C'), m, n)
+    return _diffengine.make_right_matmul(
+        param_node, child, 'dense', A.flatten(order='C'), m, n)
 
 
 
@@ -92,6 +92,7 @@ def build_param_dict(inverse_data):
     n_vars = inverse_data.x_length
     param_dict = {}
     for param_id, offset in inverse_data.param_id_map.items():
+        # this is needed to not get key errors with Constants.
         if param_id not in inverse_data.param_shapes:
             continue
         d1, d2 = normalize_shape(inverse_data.param_shapes[param_id])

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

@@ -59,29 +59,19 @@ def _extract_flat_indices_from_special_index(expr):
 
 
 def _convert_rel_entr(expr, children):
-    """Convert rel_entr(x, y) = x * log(x/y) elementwise.
+    """Convert rel_entr(x, y) = x * log(x/y).
 
-    Uses specialized functions based on argument shapes:
-    - Both scalar or both same size: make_rel_entr (elementwise)
-    - First arg vector, second scalar: make_rel_entr_vector_scalar
-    - First arg scalar, second vector: make_rel_entr_scalar_vector
+    The C engine auto-dispatches based on argument dimensions
+    (elementwise, vector-scalar, or scalar-vector).
     """
-    x_arg, y_arg = expr.args
-    x_size = x_arg.size
-    y_size = y_arg.size
-
-    # Determine which variant to use based on sizes
-    if x_size == y_size:
-        return _diffengine.make_rel_entr(children[0], children[1])
-    elif x_size > 1 and y_size == 1:
-        return _diffengine.make_rel_entr_vector_scalar(children[0], children[1])
-    elif x_size == 1 and y_size > 1:
-        return _diffengine.make_rel_entr_scalar_vector(children[0], children[1])
-    else:
+    x_size = expr.args[0].size
+    y_size = expr.args[1].size
+    if x_size > 1 and y_size > 1 and x_size != y_size:
         raise ValueError(
-            f"rel_entr requires arguments to be either both scalars, both same size, "
-            f"or one scalar and one vector. Got sizes: x={x_size}, y={y_size}"
+            f"rel_entr requires compatible argument sizes. "
+            f"Got: x={x_size}, y={y_size}"
         )
+    return _diffengine.make_rel_entr(children[0], children[1])
 
 
 def _convert_quad_form(expr, children):

cvxpy/reductions/solvers/nlp_solvers/ipopt_nlpif.py

@@ -155,8 +155,11 @@ def solve_via_data(self, data, warm_start: bool, verbose: bool, solver_opts, sol
             oracles = Oracles(bounds.new_problem, verbose=verbose, use_hessian=use_hessian)
         elif 'oracles' in solver_cache:
             oracles = solver_cache['oracles']
+            # revisit updating parameters for problems with no parameters and best_of
             oracles.update_params(bounds.new_problem)
         else:
+            # do we need to update params here? Or can we ensure that they always have values on the
+            # first iteration. cvxpy ensures that parameters have values.
             oracles = Oracles(bounds.new_problem, verbose=verbose, use_hessian=use_hessian)
             solver_cache['oracles'] = oracles
 

cvxpy/reductions/solvers/nlp_solving_chain.py

@@ -178,8 +178,9 @@ def solve_nlp(problem, solver, warm_start, verbose, **kwargs):
 
     # Reuse cached Oracles across solve() calls when problem has parameters.
     # Parameter updates happen inside solve_via_data when reusing.
+    # Also initialize an empty cache for best_of.
     solver_cache = problem._solver_cache.get('NLP')
-    if solver_cache is None and problem.parameters():
+    if solver_cache is None and (problem.parameters() or "best_of" in kwargs):
         solver_cache = {}
         problem._solver_cache['NLP'] = solver_cache
 

cvxpy/tests/nlp_tests/test_best_of.py

@@ -113,3 +113,5 @@ def test_path_planning_best_of_five(self):
         prob.solve(nlp=True, best_of=3)
         all_objs = prob.solver_stats.extra_stats['all_objs_from_best_of']
         assert len(all_objs) == 3
+
+# TODO add a test that best_of actually caches the sparsity pattern between solves

cvxpy/tests/nlp_tests/test_nlp_parameters.py

@@ -19,12 +19,9 @@
 import cvxpy as cp
 from cvxpy.reductions.solvers.defines import INSTALLED_SOLVERS
 
-pytestmark = pytest.mark.skipif(
-    'IPOPT' not in INSTALLED_SOLVERS,
-    reason="IPOPT is not installed")
 
-
-class TestNLPParameters:
+@pytest.mark.skipif('IPOPT' not in INSTALLED_SOLVERS, reason='IPOPT is not installed.')
+class Test_NLP_parameters:
 
     def test_scalar_parameter(self):
         """min p * x^2 + x, analytical solution: val = -1/(4p)."""