Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

QuadraticCost Does Not Check Matrix Shape #21578

Closed
cohnt opened this issue Jun 17, 2024 · 2 comments · Fixed by #21583
Closed

QuadraticCost Does Not Check Matrix Shape #21578

cohnt opened this issue Jun 17, 2024 · 2 comments · Fixed by #21583
Assignees

Comments

@cohnt
Copy link
Contributor

cohnt commented Jun 17, 2024

What happened?

When constructing a quadratic cost, there are restrictions on the size of the input matrices Q and b. But these aren't being checked, so if a user specifies inputs of the wrong size, no error is thrown. Instead, uninitialized values will be used in the construction of the cost or constraint, leading to cryptic downstream errors.

Here's a simple example in python, demonstrating that the matrix shape is not being checked:

import numpy as np
from pydrake.all import QuadraticCost

QuadraticCost(np.eye(3), np.zeros(2), 1)

Version

b37aa69

What operating system are you using?

No response

What installation option are you using?

No response

Relevant log output

No response

@hongkai-dai hongkai-dai self-assigned this Jun 17, 2024
@hongkai-dai
Copy link
Contributor

The size is checked in the debug mode

drake/solvers/cost.h

Lines 135 to 136 in b37aa69

DRAKE_ASSERT(Q_.rows() == Q_.cols());
DRAKE_ASSERT(Q_.cols() == b_.rows());

Since pybind is built in the release mode, it bypasses the check.

@jwnimmer-tri I can activate the check in the release mode. But what would be our general strategy? It is quite hard for python users to switch to debug mode, so shall we prefer DRAKE_DEMAND to DRAKE_ASSERT in our C++ code?

@jwnimmer-tri
Copy link
Collaborator

See #7827. The functions DRAKE_DEMAND and DRAKE_ASSERT are only appropriate for Drake checking its own internal invariants. User input should always be checked with DRAKE_THROW_UNLESS (or something similar, like an if-test with a throw statement).

Ideally, any patches to this function would also change it obey https://drake.mit.edu/styleguide/cppguide.html#Inline_Functions, but if this an emergency maybe the platform reviewer will let you get away without doing that part.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants