initial attempt at dense quadform#96
Conversation
Collapse the single-use refresh_dense_Q helper into refresh_param_values_qf, drop the unused <math.h> include, remove low-value label comments, and reflow the quad_form_expr struct comments to be clang-format clean. No behavior change. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Note in the quad_form_expr comment that QJf (sparse CSC) and QJf_dense (permuted_dense) are never both used — one per node — and why they can't share a single matrix* field (the sparse symmetric products have no matrix-vtable form). Doc-only; no behavior change. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
| } | ||
| else | ||
| { | ||
| /* jacobian = 2 * (Q @ f(x))^T @ J_f */ |
There was a problem hiding this comment.
Reminder here for the future (potential previous bug). This code should be fixed as it assumes that the jacobian of the child is CSR but may not be the case.
But it might not be a bug since we do to_csr when we call csr_to_csc_fill_values. I'll think about this.
|
Very beautiful code @Transurgeon. I really like how you use the available kernels to compute BTA to multiply the matrices regardless of their matrix type. I left some minor comments above. Fix those and then I'll do one pass and merge it. Let's also make sure to add good tests on the Python/CVXPY side. (I saw that you had already started that). It's great that you're back! |
|
@dance858 I cleaned up the PR based on your review, let me know if it looks good now! |
2 participants
This PR allows quad forms to have dense and potentially parametric constant data.
The vision I had in mind was to completely mirror the code style and matrix abstractions we had for left_matmul.
I encountered this while benchmarking the diffengine backend. This is especially useful for dense quadratic objectives where P consists mostly of just one (large and dense) quadratic form.
Now there are quite significant speedups compared to the previously existing backends.
Both forward and jacobian use dynamic dispatch (I think that's what the vtable is for) based on the matrix type so there is only need to implement them once. The hessian currently is separated as the dense case only works for single variable for
x(which makes the hessian just 2*P), I think we could potentially improve it to work for generalxin which case we could also do some dynamic dispatch.If this is easy to do and something we want, then I can improve the PR to reflect that. Otherwise I think the changes here are already quite minimal and provide the speedups I was looking for to compute these quadratic objectives that arise during canonicalisation.
@dance858 please let me know if the changes make sense and are conforming to the style of our matrix abstraction. I tried my best to understand and make my own judgment, but I must rely on your expertise for this to get merged comfortably :).