Fix matrix_frac crash on a matrix argument with an ndarray P - #3462
Open
chuenchen309 wants to merge 1 commit into
Open
chuenchen309 wants to merge 1 commit into
chuenchen309 wants to merge 1 commit into
Conversation
When P is a numpy ndarray, matrix_frac took a QuadForm shortcut unconditionally, but QuadForm only accepts a vector x. So matrix_frac(X, np.eye(n)) with a matrix X raised "Invalid dimensions for arguments to quad_form" — even though the same X, P work through Constant(P), a Variable P, or P.tolist(). Restrict the shortcut to a vector x; a matrix X now falls through to MatrixFrac (which casts the ndarray P). Co-authored-by: Claude (Claude Code) <noreply@anthropic.com>
|
|
Collaborator
|
Hello! Thank you for your contribution! We cannot review it until you sign the CLA. Additionally, we require use of our PR template, even for agent drafted PRs. |
Contributor
|
Benchmarks that have stayed the same:
|
Collaborator
|
Hello, please sign the CLA so we can review your PR! |
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
cp.matrix_frac(X, P)computestr(Xᵀ P⁻¹ X), and a matrixX(shape(n, k),k > 1) is a supported, tested case. But whenPis a numpyndarray— the most natural way to pass a constant matrix, e.g.cp.matrix_frac(X, np.eye(n))— it raisesValueError: Invalid dimensions for arguments to quad_form, and aProblemusing it cannot even be constructed.The
matrix_fracwrapper takes aQuadFormshortcut wheneverPis anndarray, butQuadFormonly accepts a vectorx. The sameX, Psucceed through every other path —cp.Constant(P), aVariableP, andP.tolist()— all returning the correcttr(Xᵀ P⁻¹ X). So the failure is triggered purely by the type ofP, not the math.The fix restricts the
QuadFormshortcut to a vectorx; a matrixXfalls through toMatrixFrac, which already handles anndarrayPby casting it to aConstant. A vectorxstill uses the shortcut unchanged, and mismatched dimensions still raise the proper error.Verified:
cvxpy/tests/test_atoms.pypasses (122), the value matchesnp.trace(X.T @ inv(P) @ X)and theConstant(P)path, and a fullProblem(Minimize(cp.matrix_frac(Variable((n, k)), P)))now constructs and solves tooptimal. Added a regression test.Issue link (if applicable): none — found during atom-behavior verification.
Type of change
Contribution checklist
cvxpy/atoms/matrix_frac.pyandcvxpy/tests/test_atoms.pyare edited.)test_atoms.py: 122 passed.)Disclosure: this change was found, fixed, tested and described by an AI coding agent (Claude Code) running on this account. The account holder reviews every change and is accountable for it. The verification above is real and re-runnable from the diff. If this isn't the kind of contribution you want, say so and I'll close it.