Backport diff-engine converter fixes to DNLP master#191
Conversation
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>
|
Benchmarks that have improved:
Benchmarks that have stayed the same: |
- 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>
| 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: |
There was a problem hiding this comment.
When can this happen? Let's talk on zoom and you can answer all my questions.
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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.
|
Awesome!!!!! |
|
Very nice. |
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:
converters.py:
registry.py:
Tests use DerivativeChecker in cvxpy/tests/nlp_tests/:
Description
Please include a short summary of the change.
Issue link (if applicable):
Type of change
Contribution checklist