Skip to content

Backport diff-engine converter fixes to DNLP master#191

Merged
dance858 merged 9 commits into
masterfrom
backport/diffengine-converter-fixes
Apr 25, 2026
Merged

Backport diff-engine converter fixes to DNLP master#191
dance858 merged 9 commits into
masterfrom
backport/diffengine-converter-fixes

Conversation

@Transurgeon

Copy link
Copy Markdown
Member

Cherry-picks the converter-level fixes from feat/diffengine-dcp-backend without the DCP-backend infrastructure. These bugs affect the existing DNLP path (solve(nlp=True)) equally.

helpers.py:

  • Dense matmul helpers use A.shape directly so 1D constants retain their true shape (right-matmul previously received (1, n) and crashed).

converters.py:

  • convert_matmul: normalize 1D constants with _matmul_normalize_1d, reshape right 1D child from (1, n) to (n, 1), and pass the child capsule as param_node when the constant side contains parameters but isn't itself a direct Parameter (e.g. A.T, reshape(A, ...)) so update_params doesn't dereference unregistered memory.
  • convert_expr: when C produces (n, 1) but CVXPY shape is (n,), reshape instead of raising.

registry.py:

  • New converters: vstack, conv/convolve, upper_tri, diag_mat, DivExpression.
  • Pass-through wraps: conj, nonneg_wrap, nonpos_wrap, psd_wrap, nsd_wrap, hermitian_wrap, skew_symmetric_wrap, symmetric_wrap.
  • convert_reshape: support order='C' via transpose -> F-reshape (swapped dims) -> transpose.
  • convert_transpose: 1D is a numpy no-op; don't flip (1, n) to (n, 1).
  • convert_quad_form (scalar P): use P.is_constant() instead of isinstance(P, cp.Constant); clearer error when P has no value.

Tests use DerivativeChecker in cvxpy/tests/nlp_tests/:

  • test_matmul.py: 1D left/right/dot cases, Parameter-inside-transpose with update_params re-solve.
  • test_affine_atoms.py (new): reshape C-order, vstack, upper_tri, diag_mat, div scalar/vector, conv, convolve.

Description

Please include a short summary of the change.
Issue link (if applicable):

Type of change

  • New feature (backwards compatible)
  • New feature (breaking API changes)
  • Bug fix
  • Other (Documentation, CI, ...)

Contribution checklist

  • Add our license to new files.
  • Check that your code adheres to our coding style.
  • Write unittests.
  • Run the unittests and check that they’re passing.
  • Run the benchmarks to make sure your change doesn’t introduce a regression.

Cherry-picks the converter-level fixes from feat/diffengine-dcp-backend
without the DCP-backend infrastructure. These bugs affect the existing
DNLP path (solve(nlp=True)) equally.

helpers.py:
- Dense matmul helpers use A.shape directly so 1D constants retain their
  true shape (right-matmul previously received (1, n) and crashed).

converters.py:
- convert_matmul: normalize 1D constants with _matmul_normalize_1d,
  reshape right 1D child from (1, n) to (n, 1), and pass the child
  capsule as param_node when the constant side contains parameters but
  isn't itself a direct Parameter (e.g. A.T, reshape(A, ...)) so
  update_params doesn't dereference unregistered memory.
- convert_expr: when C produces (n, 1) but CVXPY shape is (n,), reshape
  instead of raising.

registry.py:
- New converters: vstack, conv/convolve, upper_tri, diag_mat,
  DivExpression.
- Pass-through wraps: conj, nonneg_wrap, nonpos_wrap, psd_wrap, nsd_wrap,
  hermitian_wrap, skew_symmetric_wrap, symmetric_wrap.
- convert_reshape: support order='C' via transpose -> F-reshape
  (swapped dims) -> transpose.
- convert_transpose: 1D is a numpy no-op; don't flip (1, n) to (n, 1).
- convert_quad_form (scalar P): use P.is_constant() instead of
  isinstance(P, cp.Constant); clearer error when P has no value.

Tests use DerivativeChecker in cvxpy/tests/nlp_tests/:
- test_matmul.py: 1D left/right/dot cases, Parameter-inside-transpose
  with update_params re-solve.
- test_affine_atoms.py (new): reshape C-order, vstack, upper_tri,
  diag_mat, div scalar/vector, conv, convolve.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Comment thread cvxpy/reductions/solvers/nlp_solvers/diff_engine/registry.py
@github-actions

github-actions Bot commented Apr 22, 2026

Copy link
Copy Markdown

Benchmarks that have improved:

   before           after         ratio
 [20203aec]       [235776ef]
  •     3.10±0s          2.81±0s     0.90  quantum_hilbert_matrix.QuantumHilbertMatrix.time_compile_problem
    
  •    17.1±0ms         15.4±0ms     0.90  simple_QP_benchmarks.ParametrizedQPBenchmark.time_compile_problem
    
  •     1.28±0s          1.05±0s     0.82  gini_portfolio.Cajas.time_compile_problem
    

Benchmarks that have stayed the same:

   before           after         ratio
 [20203aec]       [235776ef]
     49.5±0ms         51.1±0ms     1.03  matrix_stuffing.SmallMatrixStuffing.time_compile_problem
      793±0ms          813±0ms     1.02  matrix_stuffing.ConeMatrixStuffingBench.time_compile_problem
      991±0ms          994±0ms     1.00  finance.FactorCovarianceModel.time_compile_problem
      21.3±0s          21.3±0s     1.00  sdp_segfault_1132_benchmark.SDPSegfault1132Benchmark.time_compile_problem
      5.59±0s          5.61±0s     1.00  optimal_advertising.OptimalAdvertising.time_compile_problem
      295±0ms          296±0ms     1.00  matrix_stuffing.ParamSmallMatrixStuffing.time_compile_problem
      718±0ms          717±0ms     1.00  simple_QP_benchmarks.LeastSquares.time_compile_problem
      247±0ms          247±0ms     1.00  simple_QP_benchmarks.SimpleQPBenchmark.time_compile_problem
      1.58±0s          1.57±0s     1.00  tv_inpainting.TvInpainting.time_compile_problem
      1.48±0s          1.47±0s     0.99  matrix_stuffing.ParamConeMatrixStuffing.time_compile_problem
      4.02±0s          3.99±0s     0.99  huber_regression.HuberRegression.time_compile_problem
      10.1±0s          10.0±0s     0.99  simple_LP_benchmarks.SimpleLPBenchmark.time_compile_problem
      333±0ms          330±0ms     0.99  gini_portfolio.Yitzhaki.time_compile_problem
      4.56±0s          4.50±0s     0.99  svm_l1_regularization.SVMWithL1Regularization.time_compile_problem
      1.82±0s          1.80±0s     0.99  simple_QP_benchmarks.UnconstrainedQP.time_compile_problem
      13.4±0s          13.2±0s     0.99  finance.CVaRBenchmark.time_compile_problem
      907±0ms          893±0ms     0.98  simple_LP_benchmarks.SimpleScalarParametrizedLPBenchmark.time_compile_problem
      285±0ms          279±0ms     0.98  slow_pruning_1668_benchmark.SlowPruningBenchmark.time_compile_problem
      240±0ms          234±0ms     0.98  gini_portfolio.Murray.time_compile_problem
     24.5±0ms         23.9±0ms     0.98  high_dim_convex_plasticity.ConvexPlasticity.time_compile_problem
     15.4±0ms         15.0±0ms     0.97  simple_LP_benchmarks.SimpleFullyParametrizedLPBenchmark.time_compile_problem
      522±0ms          508±0ms     0.97  semidefinite_programming.SemidefiniteProgramming.time_compile_problem

Transurgeon and others added 2 commits April 22, 2026 15:47
- 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>
- convert_matmul: replace _matmul_param_node helper with an inline
  ternary. convert_expr already returns param_dict[id] for Parameter
  leaves, so the child capsule equals the dict lookup — the two cases
  (bare Parameter vs Parameter wrapped in affine atom) collapse to
  "pass child if arg has any parameters".
- tests: drop hessian_approximation='exact' and derivative_test='none'
  from the new tests; both are IPOPT default_options in ipopt_nlpif.py.

217 passed / 77 skipped / 0 failed in cvxpy/tests/nlp_tests/, 17/17
new tests green.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Comment thread cvxpy/reductions/solvers/nlp_solvers/diff_engine/registry.py
Comment thread cvxpy/reductions/solvers/nlp_solvers/diff_engine/converters.py Outdated
Comment thread cvxpy/reductions/solvers/nlp_solvers/diff_engine/converters.py Outdated
f"C dimensions ({d1_C}, {d2_C}) vs Python dimensions ({d1_Python}, {d2_Python})"
)
# 1D shape (n,) normalizes to (1, n) but C may produce (n, 1); reshape.
if len(expr.shape) <= 1 and d1_C * d2_C == d1_Python * d2_Python:

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When can this happen? Let's talk on zoom and you can answer all my questions.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It happens in test_matmul_1d_right_constant. So basically when we multiply with something 1d from the right we get the following: (m,n)@(n,) which gives an output of (m,) in Python. However from the C side, we treat (m,) as a row (1,m), whereas the output of the right matmul (also from the C side) actually is a column (m,1).

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

honestly this is starting to get a lot of special casing for this weird numpy broadcasting for matmul cases.. but we are starting to understand better.
To summarize, this code is really to catch the output of the right matmuls (and more generally 1d outputs that are being mistreated as rows) rather than the input.

Comment thread cvxpy/reductions/solvers/nlp_solvers/diff_engine/registry.py
Comment thread cvxpy/tests/nlp_tests/test_affine_atoms.py
Comment thread cvxpy/tests/nlp_tests/test_matmul.py
Comment thread cvxpy/reductions/solvers/nlp_solvers/diff_engine/converters.py
Comment thread cvxpy/reductions/solvers/nlp_solvers/diff_engine/registry.py
@dance858

Copy link
Copy Markdown
Collaborator

Awesome!!!!!

@dance858

Copy link
Copy Markdown
Collaborator

Very nice.

@dance858 dance858 merged commit 775133a into master Apr 25, 2026
46 of 48 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants